Creating Xpaths in JS is slower and creates JS garbage. http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/toolkit/modules/sessionstore/XPathGenerator.jsm#23 could be quite easily replaced with C++ helper method.

One thing is that in case there is ID, the current setup is already quite fast.

Perhaps Node interface could have [ChromeOnly] generateXPath().

Or Element.

Hey Mike, we should make this bug blocks bug 536910. I'll submit my patches here.

The call to XPathGenerator.generate from [1] can be a Node so I think the new method should be in the Node's interface. Can I use regular expression in nsAString? Or I have to create a new function to perform this task: [2]. Sorry for the lack of comments, I'll copy all comments from XPathGenerator.jsm later. And I forgot to change the GetPreviousSibling to GetPreviousElementSibling :P. :smaug, since you're the reporter, can you give me some feedback? [1]: http://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/FormData.jsm#201 [2]: http://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/XPathGenerator.jsm#87

I'm not sure about using nsString with std::map. What do you guys think?

Comment on attachment 8873330 [details] [diff] [review] cpp_xpath.v1.patch Review of attachment 8873330 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsINode.cpp @@ +118,5 @@ > > +class XPathGenerator > +{ > +public: > + static void quoteArgument(const nsAString& aArg, nsAString& retval) Olli probably has more useful things to say about this implementation/ port, but I think it might be a good idea to move this class to a separate header to be included here? I mean, I feel that this file is large enough as it is already, so adding another ~120 LOC is probably something we'd like to avoid when possible. @@ +125,5 @@ > + retval.Assign(NS_LITERAL_STRING("\'") + aArg + NS_LITERAL_STRING("\'")); > + } else if (!aArg.Contains('\"')) { > + retval.Assign(NS_LITERAL_STRING("\"") + aArg + NS_LITERAL_STRING("\"")); > + } else { > + // can we use regular expression on nsAString? Regardless of whether we can, I think we want to avoid it for perf reasons. @@ +232,5 @@ > + {NS_LITERAL_STRING("xul"), NS_LITERAL_STRING("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul")} > +}; > +const std::map<nsString, nsString> XPathGenerator::mNamespacePrefixes = { > + {NS_LITERAL_STRING("http://www.w3.org/1999/xhtml"), NS_LITERAL_STRING("xhtml")}, > + {NS_LITERAL_STRING("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"), NS_LITERAL_STRING("xul")} I think this reverse map can be generated & cached instead. But that aside, I wonder if Olli might know if these strings aren't available from somewhere else? ::: toolkit/modules/sessionstore/XPathGenerator.jsm @@ -21,4 @@ > * Generates an approximate XPath query to an (X)HTML node > */ > generate: function sss_xph_generate(aNode) { > - // have we reached the document node already? Well, the litmus test here would be to completely remove this module and replace its usage across the codebase with the new Node member method. That said, it'd be wise I think to add a unit test for the new method in dom/base/test/unit/.

Comment on attachment 8873330 [details] [diff] [review] cpp_xpath.v1.patch Nit, coding style for methods is that they start with capital letters. Argument names should be in form aName. Variables should be defined on their own lines. Static member variables start with lowercase s. using n prefix for some local variables is unusual. https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide std::map is slow, especially with lookups. It is probably faster to just use nsTArray, given that there usually aren't that many namespaces. Unfortunately there isn't easy and fast way to use regexp from C++. But quoteArgument is so simple that no regexp should be needed, just some find and replace. http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/xpcom/string/nsTString.h#154-195 http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/xpcom/string/nsTString.h#389-407 and other similar methods.

We have some namespaces as atoms in http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/base/nsGkAtomList.h#2388-2398 And nsIAtoms can then be used as string using http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/xpcom/ds/nsIAtom.idl#168

I don't see where mNamespacePrefixes is actually used. What is the expected usage? For mNamespaceURIs, I don't understand this code: >+ if (!mNamespaceURIs.count(nNamespaceURI)) { >+ prefix.Assign(mNamespaceURIs.at(nNamespaceURI)); >+ } count() returns 0 or 1. The body is executed when count() returns 0; at that point at() will always throw an exception (or crash in the context of no-extensions code like Firefox, I believe). What is this actually trying to do?

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10) > I don't see where mNamespacePrefixes is actually used. What is the expected > usage? > > For mNamespaceURIs, I don't understand this code: > > >+ if (!mNamespaceURIs.count(nNamespaceURI)) { > >+ prefix.Assign(mNamespaceURIs.at(nNamespaceURI)); > >+ } > > count() returns 0 or 1. The body is executed when count() returns 0; at > that point at() will always throw an exception (or crash in the context of > no-extensions code like Firefox, I believe). What is this actually trying > to do? It's my bad. It supposed to be mNamespaceURIs.count(nNamespaceURI) without !.

OK. So you really only need this for two namespace URIs, not an open-ended set? In that case, you should just check for the two relevant namespace ids, or explicitly check IsHTML()/IsXUL().

I moved the XPathGenerator to a new file. I also added two new functions IsXul/IsXHtml as suggested by Boris. The function Replace name is weird. Can you suggest a new name?

I haven't added a test case to test the function GenerateXPath yet as I'm not sure what the xpath generation rules are. Do we have any documentation about xpath generation rules used here? Or do we already have test cases for javascript's xpath generation?

> I moved the XPathGenerator to a new file That new file is not included in the attached patch.

Comment on attachment 8873876 [details] [diff] [review] cpp_xpath.p1.v2.patch Yeah, this is missing the new file.

Sorry for that mistake! What was wrong with me? Hope I'm not missing any file this time.

So what I meant by IsXUL/IsHTML was to replace this: >+ nsAutoString nodeNamespaceURI; >+ aNode->GetNamespaceURI(nodeNamespaceURI); >+ nsAutoString prefix; >+ GetPrefix(nodeNamespaceURI, prefix); and the GetPrefix/etc bits with this: nsAutoString prefix; GetPrefix(aNode, prefix); where void GetPrefix(nsINode* aNode, nsAString& aResult) { if (aNode->IsHTML()) { aResult.Assign(NS_LITERAL_STRING("xhtml")); } else if (aNode->IsXUL()) { aResult.Assign(NS_LITERAL_STRING("xul")); } }

I build locally and found some errors, so I fixed those and re-upload. I didn't add IsXul/IsHtml to class nsINode since I think that those functions are used only in this patch. I think we should only put functions in classes when they are useful in many places. But I'm not sure. The name in [1] isn't local name so I added another function to get the name attribute from element. [1]: http://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/XPathGenerator.jsm#42

> I didn't add IsXul/IsHtml to class nsINode since I think that those functions are used only in this patch. You don't have to add anything. The functions are already there and used all over the place. I guess they've been renamed to IsXULElement() and IsHTMLElement() is all. Please just use them instead of reimplementing them in a really slow way. > The name in [1] isn't local name so I added another function to get the name attribute from element. This part: >+ mozilla::dom::DOMString str; >+ elem->GetAttr(NS_LITERAL_STRING("name"), str); >+ str.ToString(aResult); Can be: elem->GetAttr(kNameSpaceID_None, nsGkAtoms::name, aResult); to avoid extra copies and whatnot.

Changed as suggested by Boris.

Comment on attachment 8874668 [details] [diff] [review] cpp_xpath.p1.v5.patch A DOM-peer's feedback is most important here.

Comment on attachment 8874669 [details] [diff] [review] cpp_xpath.p2.v5.patch Review of attachment 8874669 [details] [diff] [review]: ----------------------------------------------------------------- OK, this looks quite solid from my perspective. There's more specific test coverage in the sessionstore component, the main consumer of this feature. ::: dom/base/test/unit/test_generate_xpath.js @@ +1,4 @@ > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ A license block is not necessary for test files. @@ +21,5 @@ > + </html> > + `; > + let doc = DOMParser().parseFromString(docString, "text/html"); > + > + // test generate xpath for body nit: Please start comments with a capital and end with a dot. ::: dom/base/test/unit/xpcshell.ini @@ +51,5 @@ > [test_xmlserializer.js] > [test_cancelPrefetch.js] > [test_chromeutils_base64.js] > +[test_generate_xpath.js] > +head = head_xml.js \ No newline at end of file Good find!

Comment on attachment 8874668 [details] [diff] [review] cpp_xpath.p1.v5.patch Review of attachment 8874668 [details] [diff] [review]: ----------------------------------------------------------------- I'll take a look at the changes I requested during the review round. Thanks! ::: dom/base/XPathGenerator.cpp @@ +1,1 @@ > +#include "XPathGenerator.h" Missing license header. ::: toolkit/modules/sessionstore/XPathGenerator.jsm @@ -7,3 @@ > this.EXPORTED_SYMBOLS = ["XPathGenerator"]; > > this.XPathGenerator = { Please blast this whole file from the tree and all its references. We don't need it anymore, thanks to your work in dom/

Comment on attachment 8874668 [details] [diff] [review] cpp_xpath.p1.v5.patch >diff --git a/dom/base/XPathGenerator.cpp b/dom/base/XPathGenerator.cpp >new file mode 100644 >--- /dev/null >+++ b/dom/base/XPathGenerator.cpp this file is missing the license. >@@ -0,0 +1,191 @@ >+#include "XPathGenerator.h" >+ >+#include "nsGkAtoms.h" >+#include "Element.h" >+#include "nsTArray.h" >+ >+/** >+ * Check whether a character is a non-word character. A non-word character is a >+ * character that isn't in ('a'..'z') or in ('A'..'Z') or an underscore. >+ * */ */ Why no numbers? >+ >+/** >+ * Get the prefix according to the given namespace and assign the result to aResult. >+ * */ >+void GetPrefix(const nsINode* aNode, nsAString& aResult) >+{ >+ if (aNode->IsXULElement()) { >+ aResult.Assign(NS_LITERAL_STRING("xul")); >+ } else if (aNode->IsHTMLElement()) { >+ aResult.Assign(NS_LITERAL_STRING("xhtml")); >+ } >+} >+ >+void GetAttributeName(const nsINode* aNode, nsAString& aResult) GetNameAttribute >+/** >+ * Replace all sequences of '\'' in a string with "\',\"$&\",\'" >+ * */ Do what? Why we want to use $& here? That is a special value in JS replace() method. >diff --git a/dom/base/XPathGenerator.h b/dom/base/XPathGenerator.h >new file mode 100644 >--- /dev/null >+++ b/dom/base/XPathGenerator.h this file is missing the license. > this.XPathGenerator = { >- // these two hashes should be kept in sync >+ // this should be kept in sync with dom/base/XPathGenerator.cpp A bit unclear to me what should be kept in sync

Comment on attachment 8874669 [details] [diff] [review] cpp_xpath.p2.v5.patch This needs some changes too after the patch has been fixed.

Changed with comments addressed. The kept in sync comment was there because I used std::map. Since I changed to use IsXul/IsHtml instead, that comment is no longer needed.

Comment on attachment 8875607 [details] [diff] [review] cpp_xpath.p1.v6.patch Review of attachment 8875607 [details] [diff] [review]: ----------------------------------------------------------------- The JS changes look good to me, I'd just like to see the final patch once more just to verify. I'd like to leave the CPP changes to Olli, because well, obvious reasons. ::: dom/base/XPathGenerator.cpp @@ +35,5 @@ > + const char16_t* end = aStr.EndReading(); > + for (; cur < end; ++cur) { > + if (IsNonWordCharacter(*cur)) { > + return true; > + } nit: trailing space (pro-tip: you can configure your editor to this automatically for you!) ::: dom/base/XPathGenerator.h @@ +14,5 @@ > +public: > + /** > + * Return a properly quoted string to insert into an XPath > + * */ > + static void QuoteArgument(const nsAString& aArg, nsAString& aResult); nit: trailing whitespace. @@ +24,5 @@ > + > + /** > + * Generate an approximate XPath query to an (X)HTML node > + * */ > + static void Generate(const nsINode* aNode, nsAString& aResult); nit: trailing whitespace. ::: toolkit/modules/sessionstore/FormData.jsm @@ +95,4 @@ > * This module's internal API. > */ > var FormDataInternal = { > + namespaceURIs: { nit: Please remove this superfluous spaces after the colon here. @@ +102,5 @@ > + > + /** > + * Resolves an XPath query generated by node.generateXPath. > + */ > + resolve: function sss_xph_resolve(aDocument, aQuery) { Please remove 'sss_xph_resolve' and write this as `resolve(document, query) {` @@ +110,5 @@ > + > + /** > + * Namespace resolver for the above XPath resolver. > + */ > + resolveNS: function sss_xph_resolveNS(aPrefix) { Please remove 'sss_xph_resolveNS' and write this as `resolveNS(prefix) {` @@ +115,5 @@ > + return this.namespaceURIs[aPrefix] || null; > + }, > + > + /** > + * @returns an XPath query to all savable form field nodes nit: please align the asterisks properly here. @@ +117,5 @@ > + > + /** > + * @returns an XPath query to all savable form field nodes > + */ > + get restorableFormNodes() { nit: can you change the name to 'restorableFormNodesXPath'?

Comment on attachment 8875608 [details] [diff] [review] cpp_xpath.p2.v6.patch Review of attachment 8875608 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the JS test.

Updated as Mike's requests.

Comment on attachment 8876026 [details] [diff] [review] cpp_xpath.p2.v7.patch >+TEST(TestXPathGenerator, TestQuoteArgumentWithSingleAndDoubleQuote) >+{ >+ nsAutoString arg; >+ arg.Assign(NS_LITERAL_STRING("\'testing\"")); >+ >+ nsAutoString expectedResult; >+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\",\'testing\"\')")); Why this has \'\' as the first param? Is that just a side effect? Why the need for \ before ' there? >+TEST(TestXPathGenerator, TestQuoteArgumentWithDoubleQuoteAndASequenceOfSingleQuote) >+{ >+ nsAutoString arg; >+ arg.Assign(NS_LITERAL_STRING("\'\'\'\'testing\"")); >+ >+ nsAutoString expectedResult; >+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\'\'\'\",\'testing\"\')")); Also here. What is the first \'\' ? >+function test_generate_xpath() >+{ >+ let docString = ` >+ <html> >+ <body> >+ <label><input type="checkbox" id="input1" />Input 1</label> >+ <label><input type="checkbox" />Input 2</label> >+ </body> >+ </html> >+ `; >+ let doc = DOMParser().parseFromString(docString, "text/html"); >+ >+ // Test generate xpath for body. >+ do_print("Test generate xpath for body node"); >+ let body = doc.getElementsByTagName("body")[0]; >+ let bodyXPath = body.generateXPath(); >+ let bodyExpXPath = "/xhtml:html/xhtml:body"; >+ equal(bodyExpXPath, bodyXPath, " xpath generated for body"); >+ >+ // Test generate xpath for input with id. >+ do_print("Test generate xpath for input with id"); >+ let inputWithId = doc.getElementById("input1"); >+ let inputWithIdXPath = inputWithId.generateXPath(); >+ let inputWithIdExpXPath = "//xhtml:input[@id='input1']"; >+ equal(inputWithIdExpXPath, inputWithIdXPath, " xpath generated for input with id"); >+ >+ // Test generate xpath for input without id. >+ do_print("Test generate xpath for input without id"); >+ let inputNoId = doc.getElementsByTagName("input")[1]; >+ let inputNoIdXPath = inputNoId.generateXPath(); >+ let inputNoIdExpXPath = "/xhtml:html/xhtml:body/xhtml:label[2]/xhtml:input"; >+ equal(inputNoIdExpXPath, inputNoIdXPath, " xpath generated for input without id"); I'd really prefer to have tests for more complicated cases. Like when id attribute has ' or " both in it. Say, what kind of xpath is generated if one first calls inputWithId.setAttribute("id", "'\"foo\"'"); and then generates xpath for it.

Comment on attachment 8876025 [details] [diff] [review] cpp_xpath.p1.v7.patch ># HG changeset patch ># User Beekill95 <nnn_bikiu0707@yahoo.com> ># Date 1496886574 -25200 ># Thu Jun 08 08:49:34 2017 +0700 ># Node ID a3796a0d88b527d3203cacd52eed599be0236c04 ># Parent 4541134e973a6bd5e667a603e844854c8e5361da >Move XPath generation to Node's interface and move all remaining XPathGenerator.jsm functions to FormData > >MozReview-Commit-ID: 44mjlyF3pX2 > >diff --git a/dom/base/XPathGenerator.cpp b/dom/base/XPathGenerator.cpp >new file mode 100644 >--- /dev/null >+++ b/dom/base/XPathGenerator.cpp >@@ -0,0 +1,191 @@ >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set ts=8 sts=2 et sw=2 tw=80: */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "XPathGenerator.h" >+ >+#include "nsGkAtoms.h" >+#include "Element.h" >+#include "nsTArray.h" >+ >+/** >+ * Check whether a character is a non-word character. A non-word character is a >+ * character that isn't in ('a'..'z') or in ('A'..'Z') or a number or an underscore. >+ * */ >+bool IsNonWordCharacter(const char16_t& aChar) >+{ >+ if (((char16_t('A') <= aChar) && (aChar <= char16_t('Z'))) || >+ ((char16_t('a') <= aChar) && (aChar <= char16_t('z'))) || >+ ((char16_t('0') <= aChar) && (aChar <= char16_t('9'))) || >+ (aChar == char16_t('_'))) { >+ return false; >+ } else { >+ return true; >+ } >+} >+ >+/** >+ * Check whether a string contains a non-word character. >+ * */ >+bool ContainNonWordCharacter(const nsAString& aStr) >+{ >+ const char16_t* cur = aStr.BeginReading(); >+ const char16_t* end = aStr.EndReading(); >+ for (; cur < end; ++cur) { >+ if (IsNonWordCharacter(*cur)) { >+ return true; >+ } >+ } >+ return false; >+} >+ >+/** >+ * Get the prefix according to the given namespace and assign the result to aResult. >+ * */ >+void GetPrefix(const nsINode* aNode, nsAString& aResult) >+{ >+ if (aNode->IsXULElement()) { >+ aResult.Assign(NS_LITERAL_STRING("xul")); >+ } else if (aNode->IsHTMLElement()) { >+ aResult.Assign(NS_LITERAL_STRING("xhtml")); >+ } >+} >+ >+void GetNameAttribute(const nsINode* aNode, nsAString& aResult) >+{ >+ if (aNode->HasName()) { >+ const Element* elem = aNode->AsElement(); >+ elem->GetAttr(kNameSpaceID_None, nsGkAtoms::name, aResult); >+ } >+} >+ >+/** >+ * Put all sequences of ' in a string in between '," and ",' . >+ * >+ * For example, a string 'a'' will return result ',"'",'a',"''",' >+ * */ >+void SurroundSingleQuotes(const nsAString& aStr, nsAString& aResult) I'm having hard time to understand why this returns a string which starts with ', and ends with ,'. The only caller then needs to do NS_LITERAL_STRING("concat(\'") + result + NS_LITERAL_STRING("\')") >+ const char16_t* nonQuoteBeginPtr = nullptr; >+ const char16_t* quoteBeginPtr = nullptr; >+ for (; cur < end; ++cur) { >+ if (char16_t('\'') == *cur) { >+ if (nonQuoteBeginPtr) { >+ aResult.Append(nonQuoteBeginPtr, cur - nonQuoteBeginPtr); >+ nonQuoteBeginPtr = nullptr; >+ } >+ if (!quoteBeginPtr) { >+ aResult.Append(NS_LITERAL_STRING("\',\"")); There is a tab here, use spaces for indentation >+ quoteBeginPtr = cur; >+ } >+ } else { >+ if (!nonQuoteBeginPtr) { >+ nonQuoteBeginPtr = cur; You have some tabs here. Use spaces for indentation >+ } >+ if (quoteBeginPtr) { >+ aResult.Append(quoteBeginPtr, cur - quoteBeginPtr); >+ aResult.Append(NS_LITERAL_STRING("\",\'")); >+ quoteBeginPtr = nullptr; tabs also here.

Comment on attachment 8876025 [details] [diff] [review] cpp_xpath.p1.v7.patch Review of attachment 8876025 [details] [diff] [review]: ----------------------------------------------------------------- Oops! ::: toolkit/modules/sessionstore/FormData.jsm @@ +170,4 @@ > */ > collect({document: doc}) { > let formNodes = doc.evaluate( > + this.restorableFormNodes, You probably want to change the name here too! Did you test this?

Comment on attachment 8876026 [details] [diff] [review] cpp_xpath.p2.v7.patch Review of attachment 8876026 [details] [diff] [review]: ----------------------------------------------------------------- I think I already r+'d this, so you don't need to re-request review from me again.

>>+/** >>+ * Put all sequences of ' in a string in between '," and ",' . >>+ * >>+ * For example, a string 'a'' will return result ',"'",'a',"''",' >>+ * */ >>+void SurroundSingleQuotes(const nsAString& aStr, nsAString& aResult) >I'm having hard time to understand why this returns a string >which starts with ', and ends with ,'. >The only caller then needs to do >NS_LITERAL_STRING("concat(\'") + result + NS_LITERAL_STRING("\')") Actually, the return string doesn't need to starts with ', and ends with ,'. If we take the string a'a, the return string should be a',"'",'a. We return a string which starts with ', and ends with ,' when the string has single quote at both the beginning and the end. >Oops! >::: toolkit/modules/sessionstore/FormData.jsm >@@ +170,4 @@ >> */ >> collect({document: doc}) { >> let formNodes = doc.evaluate( >> + this.restorableFormNodes, >You probably want to change the name here too! Did you test this? I though it was a simple change so I didn't test it.

>>+TEST(TestXPathGenerator, TestQuoteArgumentWithSingleAndDoubleQuote) >>+{ >>+ nsAutoString arg; >>+ arg.Assign(NS_LITERAL_STRING("\'testing\"")); >>+ >>+ nsAutoString expectedResult; >>+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\",\'testing\"\')")); >Why this has \'\' as the first param? >Is that just a side effect? >Why the need for \ before ' there? When there is a single quote(s) at the beginning of a string, we put that single quote(s) in between '," and ",'. After that, the return string is then put inside concat(' and '). So the first param should be \'\'. Isn't \ is needed so the compiler can treat ' as normal character? Mike, I know you have give me an r+, but I added some testcases as Olli's request. I think you should also take a look at this patch.

Comment on attachment 8876605 [details] [diff] [review] cpp_xpath.p1.v8.patch Review of attachment 8876605 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the JS bits!

(In reply to Bao Quan [:beekill] from comment #45) > Created attachment 8876607 [details] [diff] [review] > cpp_xpath.p2.v8.patch > > >>+TEST(TestXPathGenerator, TestQuoteArgumentWithSingleAndDoubleQuote) > >>+{ > >>+ nsAutoString arg; > >>+ arg.Assign(NS_LITERAL_STRING("\'testing\"")); > >>+ > >>+ nsAutoString expectedResult; > >>+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\",\'testing\"\')")); > >Why this has \'\' as the first param? > >Is that just a side effect? > >Why the need for \ before ' there? > > When there is a single quote(s) at the beginning of a string, we put that > single quote(s) in between '," and ",'. I don't understand this: between '," and ",' > After that, the return string is > then put inside concat(' and '). So the first param should be \'\'. Why? single quote is inside \"<here>\". Why we need the \'\'? That is just '' in JS then and passed as empty string to xpath method concat().

Comment on attachment 8876607 [details] [diff] [review] cpp_xpath.p2.v8.patch >+ nsAutoString expectedResult; >+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\",\'testing\"\')")); It is still surprising that we have \'\' as first param to concat >+TEST(TestXPathGenerator, TestQuoteArgumentWithDoubleQuoteAndASequenceOfSingleQuote) >+{ >+ nsAutoString arg; >+ arg.Assign(NS_LITERAL_STRING("\'\'\'\'testing\"")); >+ >+ nsAutoString expectedResult; >+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\'\'\'\",\'testing\"\')")); Also here, surprising to have \'\' >+TEST(TestXPathGenerator, TestQuoteArgumentWithDoubleQuoteAndTwoSequencesOfSingleQuote) >+{ >+ nsAutoString arg; >+ arg.Assign(NS_LITERAL_STRING("\'\'\'\'testing\'\'\'\'\'\'\"")); >+ >+ nsAutoString expectedResult; >+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\'\'\'\",\'testing\',\"\'\'\'\'\'\'\",\'\"\')")); And here. Why we need to extra \'\' But I guess I can live with extra empty string

Comment on attachment 8876605 [details] [diff] [review] cpp_xpath.p1.v8.patch >+bool IsNonWordCharacter(const char16_t& aChar) >+{ >+ if (((char16_t('A') <= aChar) && (aChar <= char16_t('Z'))) || >+ ((char16_t('a') <= aChar) && (aChar <= char16_t('z'))) || >+ ((char16_t('0') <= aChar) && (aChar <= char16_t('9'))) || You have tab here. Use spaces for indentation >+ * Put all sequences of ' in a string in between '," and ",' . >+ * >+ * For example, a string 'a'' will return result ',"'",'a',"''",' >+ * */ >+void SurroundSingleQuotes(const nsAString& aStr, nsAString& aResult) >+{ >+ const char16_t* cur = aStr.BeginReading(); >+ const char16_t* end = aStr.EndReading(); >+ >+ const char16_t* nonQuoteBeginPtr = nullptr; >+ const char16_t* quoteBeginPtr = nullptr; >+ for (; cur < end; ++cur) { >+ if (char16_t('\'') == *cur) { >+ if (nonQuoteBeginPtr) { >+ aResult.Append(nonQuoteBeginPtr, cur - nonQuoteBeginPtr); >+ nonQuoteBeginPtr = nullptr; >+ } There are some tabs here (also elsewhere.). Use spaces, never tabs. Please fix all the cases. >+void XPathGenerator::QuoteArgument(const nsAString& aArg, nsAString& aResult) >+{ >+ if (!aArg.Contains('\'')) { >+ aResult.Assign(NS_LITERAL_STRING("\'") + aArg + NS_LITERAL_STRING("\'")); >+ } else if (!aArg.Contains('\"')) { >+ aResult.Assign(NS_LITERAL_STRING("\"") + aArg + NS_LITERAL_STRING("\"")); >+ } else { >+ nsAutoString result; >+ SurroundSingleQuotes(aArg, result); >+ aResult.Assign(NS_LITERAL_STRING("concat(\'") + result + NS_LITERAL_STRING("\')")); This is still very confusing. Why the called method doesn't just return the right string, even with all that concat( stuff. Right now one needs to guess how to use SurroundSingleQuotes, that concat(' needs to be prepended to it and ') appended . >+void XPathGenerator::Generate(const nsINode* aNode, nsAString& aResult) >+{ >+ if (!aNode->GetParentNode()) { >+ aResult.Assign(NS_LITERAL_STRING("")); aResult.Truncate(); or aResult = EmptyString(); >+ >+ if (aNode->HasID()) { >+ // this must be an element >+ const Element* elem = aNode->AsElement(); >+ nsAutoString elemId, quotedArgument; Nit, variable definitions to separate lines please. >+ elem->GetId(elemId); >+ QuoteArgument(elemId, quotedArgument); >+ aResult.Assign(NS_LITERAL_STRING("//") + tag + NS_LITERAL_STRING("[@id=") + >+ quotedArgument + NS_LITERAL_STRING("]")); Nit, missing space before quotedArgument. >+ int32_t count = 1; >+ nsAutoString nodeNameAttribute; >+ GetNameAttribute(aNode, nodeNameAttribute); >+ for (const Element* e = aNode->GetPreviousElementSibling(); e; e = e->GetPreviousElementSibling()) { >+ nsAutoString eNamespaceURI; Nit, e is not any variable prefix, so this looks a bit unusual. perhaps use just namespaceURI or elementNamespaceURI >+ e->GetNamespaceURI(eNamespaceURI); >+ nsAutoString eNameAttribute; similar here Still some nits to fix.

Changed as Olli's suggestions.

(In reply to Olli Pettay [:smaug] from comment #47) > (In reply to Bao Quan [:beekill] from comment #45) > > Created attachment 8876607 [details] [diff] [review] > > cpp_xpath.p2.v8.patch > > > > >>+TEST(TestXPathGenerator, TestQuoteArgumentWithSingleAndDoubleQuote) > > >>+{ > > >>+ nsAutoString arg; > > >>+ arg.Assign(NS_LITERAL_STRING("\'testing\"")); > > >>+ > > >>+ nsAutoString expectedResult; > > >>+ expectedResult.Assign(NS_LITERAL_STRING("concat(\'\',\"\'\",\'testing\"\')")); > > >Why this has \'\' as the first param? > > >Is that just a side effect? > > >Why the need for \ before ' there? > > > > When there is a single quote(s) at the beginning of a string, we put that > > single quote(s) in between '," and ",'. > I don't understand this: between '," and ",' What I meant is if we have a sequence of ' like '''', then '," is prepended and "," appended to produce a string like this: ',"''''",' > > After that, the return string is > > then put inside concat(' and '). So the first param should be \'\'. > Why? single quote is inside \"<here>\". Why we need the \'\'? That is just > '' in JS then > and passed as empty string to xpath method concat(). The empty '' as the first param is because the function SurroundSingleQuotes doesn't differentiate between single quotes in the beginning or in the middle of a string. It always put single quotes in between '," and ",'. And the string: concat(' is prepended to the return string. So if we have a string: ''a then the result will be concat('',"''",'a'), and a string a'' will result in concat('a',"''",'').

Comment on attachment 8877843 [details] [diff] [review] cpp_xpath.p1.v9.patch Have you run this through tryserver?

(In reply to Olli Pettay [:smaug] from comment #52) > Comment on attachment 8877843 [details] [diff] [review] > cpp_xpath.p1.v9.patch > > Have you run this through tryserver? I've just pushed to try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=391bc497551ca49ebf97f31b2beab20eeae8851f

There is some evidence in bug 1373672 to suggest that this may help with the Speedometer benchmark. Any chance you could please run a build with and without the patches here on the Speedometer benchmark here http://speedometer2.benj.me/ for a quick test to see if it changes the benchmark score you get at all? Thanks!

Sorry for the noise, looks like this bug may not help with the benchmark after all.

Fixed ESlint errors and error "this is undefined" in function resolveNS. Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=308d5d4c80fd478b76965d193b0dfb6e655b9af9

Requesting checkin for both patches (p1 and p2, in that order); both are 'r=smaug,mikedeboer'. Thank you!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4230d2eab5f Move XPath generation to Node's interface and move all remaining XPathGenerator.jsm functions to FormData. r=smaug, r=mikedeboer https://hg.mozilla.org/integration/mozilla-inbound/rev/6660b55684d3 Add testcases for C++ XPathGenerator's functions. r=smaug, r=mikedeboer

Backed out for failing asan-fuzzing (Bof) job, undeclared identifier TaskCategory at nsContentPolicy.cpp:142: https://hg.mozilla.org/integration/mozilla-inbound/rev/8962204c2d586467b2e46764c78b21b6b447af29 https://hg.mozilla.org/integration/mozilla-inbound/rev/65cb7c52f94b640a840f2d990e18c19868392d04 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6660b55684d3925aa6fdbe6fc001df7c4cd72152&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=108207043&repo=mozilla-inbound /home/worker/workspace/build/src/dom/base/nsContentPolicy.cpp:142:59: error: use of undeclared identifier 'TaskCategory'; did you mean 'mozilla::TaskCategory'? /home/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:1235:11: error: static_cast from 'nsINode *' to 'mozilla::dom::HTMLBodyElement *', which are not related by inheritance, is not allowed

Beekill, sorry this patch had to be backed out, see the previous comment. Please fix the issue and submit an updated patch.

I'm not sure what happened but I built the patches locally and everything was fine. I triggered a try build [1]. I uploaded the second part, in case something is wrong with cpp_xpath.p2.v8.patch. :aryx, can you do a push with the second part I just uploaded? [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22067470b21ce40a8bc922aae40856e1d2167978

Olli, is it possible for you to help point Quan in the right direction here on how to fix the BOF failure? You might've had to deal with fuzzing bugs before?

So isn't comment 59 just about some missing #includes? They may not show up on all the built setup because of unified building.

Yup, sure seems like it. Quan, can you take a look?

was the issue coming from bug 1373536 after all?

or perhaps that triggered the same issue. Look at the last patch there.

Added some includes as Olli suggested.

Comment on attachment 8880251 [details] [diff] [review] cpp_xpath.p3.v10.patch No need for this, if the #include issues will be handled in bug 1373536. Just need to wait for that to land before landing patch in this bug.

(In reply to Olli Pettay [:smaug] from comment #68) > Comment on attachment 8880251 [details] [diff] [review] > cpp_xpath.p3.v10.patch > > No need for this, if the #include issues will be handled in bug 1373536. > Just need to wait for that to land before landing patch in this bug. Ok, cool! But I don't understand why my patch causes the problem? It shown errors in files nsContentPolicy.cpp and nsFrameLoader.cpp, both of which my patch doesn't touch.

Because of unified builds. Several .cpp files get merged together and compiled in that way (too speed up built process). if #includes in some other file aren't right, the compilation may fail. Not your fault.

Attached new patches with updated commit message and fix weird indentation. Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55caef30307d8c2c0dbaf5c2c1491d3ff43f6f43

Re-requesting checkin for both patches (p1 and p2, in that order). Thanks!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9342f0d949f1 Part 1: Move XPath generation to Node's interface and move all remaining XPathGenerator.jsm functions to FormData. r=mikedeboer, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/4b95aa66ba1a Part 2: Add testcases for C++ XPathGenerator's functions. r=mikedeboer,r=smaug

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8c574e38c77d7a2f1616e2b67fdb854e361a91ef&filter-searchStr=Linux+x64+asan+Executed+by+TaskCluster+build-linux64-asan-fuzzing%2Fopt+tc(Bof) for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111634023&repo=mozilla-inbound from this or the other bug

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e80cadf541c Backed out changeset 4b95aa66ba1a

Added nsIContentParent header file to nsFocusManager to fix linux64-asan-fuzzing build failed. Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95a0475ece3692bca0b6ac38cb1e4a69ca0b9302

Re-requesting checkin for all three patches (p1, p2 and p3, in that order). Thanks!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/72ef9aacfb91 Part 1: Move XPath generation to Node's interface and move all remaining XPathGenerator.jsm functions to FormData. r=mikedeboer, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ab638d4180 Part 2: Add testcases for C++ XPathGenerator's functions. r=mikedeboer,r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/098f29ddc693 Part 3: Include missing header file to nsFocusManager.cpp to fixed linux64-asan-fuzzing build failed. r=smaug

https://hg.mozilla.org/mozilla-central/rev/72ef9aacfb91 https://hg.mozilla.org/mozilla-central/rev/d1ab638d4180 https://hg.mozilla.org/mozilla-central/rev/098f29ddc693