1362330 - Add a C++ helper to create XPaths for session restore

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Or Element.

Comment 4

[Bao Quan [:beekill]]

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

Comment 5

[Bao Quan [:beekill]]

Attachment: cpp_xpath.v1.patch

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

Comment 6

[Bao Quan [:beekill]]

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

Comment 7

[Mike de Boer [:mikedeboer]]

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 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Bao Quan [:beekill]]

(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 !.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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().

Comment 13

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v2.patch

Comment 14

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v2.patch

Comment 15

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v2.patch

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?

Comment 16

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v2.patch

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I moved the XPathGenerator to a new file

That new file is not included in the attached patch.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873876 [details] [diff] [review]
cpp_xpath.p1.v2.patch

Yeah, this is missing the new file.

Comment 19

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v3.patch

Comment 20

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v3.patch

Sorry for that mistake! What was wrong with me?

Hope I'm not missing any file this time.

Comment 21

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v3.patch

Comment 22

[Boris Zbarsky [:bzbarsky]]

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"));
    }
  }

Comment 23

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p3.v3.patch

Comment 24

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v4.patch

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

Comment 25

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v4.patch

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 27

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v5.patch

Changed as suggested by Boris.

Comment 28

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v5.patch

Comment 29

[Mike de Boer [:mikedeboer]]

Comment on attachment 8874668 [details] [diff] [review]
cpp_xpath.p1.v5.patch

A DOM-peer's feedback is most important here.

Comment 30

[Mike de Boer [:mikedeboer]]

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 31

[Mike de Boer [:mikedeboer]]

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 32

[Olli Pettay [:smaug][[e-mail]]]

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 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8874669 [details] [diff] [review]
cpp_xpath.p2.v5.patch

This needs some changes too after the patch has been fixed.

Comment 34

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v6.patch

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 35

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v6.patch

Comment 36

[Mike de Boer [:mikedeboer]]

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 37

[Mike de Boer [:mikedeboer]]

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.

Comment 38

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v7.patch

Updated as Mike's requests.

Comment 39

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v7.patch

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

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 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876025 [details] [diff] [review]
cpp_xpath.p1.v7.patch

># HG changeset patch
># User Beekill95 <[e-mail]>
># 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 42

[Mike de Boer [:mikedeboer]]

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 43

[Mike de Boer [:mikedeboer]]

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.

Comment 44

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v8.patch

>>+/**
>>+ * 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.

Comment 45

[Bao Quan [:beekill]]

Attachment: 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 ",'. 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 46

[Mike de Boer [:mikedeboer]]

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!

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(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 48

[Olli Pettay [:smaug][[e-mail]]]

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 49

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 50

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v9.patch

Changed as Olli's suggestions.

Comment 51

[Bao Quan [:beekill]]

(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 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8877843 [details] [diff] [review]
cpp_xpath.p1.v9.patch

Have you run this through tryserver?

Comment 53

[Bao Quan [:beekill]]

(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

Comment 54

[(no longer active)]

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!

Comment 55

[(no longer active)]

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

Comment 56

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v10.patch

Fixed ESlint errors and error "this is undefined" in function resolveNS.

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=308d5d4c80fd478b76965d193b0dfb6e655b9af9

Comment 57

[Mike de Boer [:mikedeboer]]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
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

Comment 59

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 61

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v10.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

Comment 62

[Mike de Boer [:mikedeboer]]

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?

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 64

[Mike de Boer [:mikedeboer]]

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

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

was the issue coming from bug 1373536 after all?

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 67

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p3.v10.patch

Added some includes as Olli suggested.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 69

[Bao Quan [:beekill]]

(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.

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 71

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v10.patch

Comment 72

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p2.v10.patch

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

Comment 73

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p1.v10.patch

Comment 74

[Mike de Boer [:mikedeboer]]

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

Comment 75

[Pulsebot]

Pushed by [e-mail]:
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

Comment 76

[Carsten Book [:Tomcat]]

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

Comment 77

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e80cadf541c
Backed out changeset 4b95aa66ba1a

Comment 78

[Bao Quan [:beekill]]

Attachment: cpp_xpath.p3.v10.patch

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

Comment 79

[Mike de Boer [:mikedeboer]]

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

Comment 80

[Pulsebot]

Pushed by [e-mail]:
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

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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