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

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: beekill)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 24 obsolete attachments)

8.86 KB, patch
Details | Diff | Splinter Review
17.85 KB, patch
Details | Diff | Splinter Review
728 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
One thing is that in case there is ID, the current setup is already quite fast.
(Reporter)

Comment 2

2 years ago
Perhaps Node interface could have [ChromeOnly]
generateXPath().
(Reporter)

Comment 3

2 years ago
Or Element.
(Assignee)

Comment 4

2 years ago
Hey Mike, we should make this bug blocks bug 536910. I'll submit my patches here.
Flags: needinfo?(mdeboer)
Assignee: nobody → nnn_bikiu0707
Blocks: 536910
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
(Assignee)

Comment 5

2 years ago
Created attachment 8873330 [details] [diff] [review]
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
Attachment #8873330 - Flags: feedback?(mdeboer)
Attachment #8873330 - Flags: feedback?(bugs)
(Assignee)

Comment 6

2 years ago
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/.
Attachment #8873330 - Flags: feedback?(mdeboer)
(Reporter)

Comment 8

2 years ago
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.
Attachment #8873330 - Flags: feedback?(bugs) → feedback+
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?
(Assignee)

Comment 11

2 years ago
(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().
(Assignee)

Comment 13

2 years ago
Created attachment 8873868 [details] [diff] [review]
cpp_xpath.p1.v2.patch
Attachment #8873330 - Attachment is obsolete: true
Attachment #8873868 - Flags: feedback?
(Assignee)

Comment 14

2 years ago
Created attachment 8873869 [details] [diff] [review]
cpp_xpath.p1.v2.patch
Attachment #8873868 - Attachment is obsolete: true
Attachment #8873868 - Flags: feedback?
Attachment #8873869 - Flags: feedback?
(Assignee)

Comment 15

2 years ago
Created attachment 8873876 [details] [diff] [review]
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?
Attachment #8873869 - Attachment is obsolete: true
Attachment #8873869 - Flags: feedback?
Attachment #8873876 - Flags: feedback?(mdeboer)
Attachment #8873876 - Flags: feedback?(bugs)
(Assignee)

Comment 16

2 years ago
Created attachment 8873878 [details] [diff] [review]
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?
Attachment #8873878 - Flags: feedback?(mdeboer)
Attachment #8873878 - Flags: feedback?(bugs)
> I moved the XPathGenerator to a new file

That new file is not included in the attached patch.
(Reporter)

Comment 18

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

Yeah, this is missing the new file.
Attachment #8873876 - Flags: feedback?(mdeboer)
Attachment #8873876 - Flags: feedback?(bugs)
(Assignee)

Comment 19

2 years ago
Created attachment 8874059 [details] [diff] [review]
cpp_xpath.p1.v3.patch
Attachment #8873876 - Attachment is obsolete: true
Attachment #8873878 - Attachment is obsolete: true
Attachment #8873878 - Flags: feedback?(mdeboer)
Attachment #8873878 - Flags: feedback?(bugs)
Attachment #8874059 - Flags: feedback?(mdeboer)
(Assignee)

Comment 20

2 years ago
Created attachment 8874062 [details] [diff] [review]
cpp_xpath.p1.v3.patch

Sorry for that mistake! What was wrong with me?

Hope I'm not missing any file this time.
Attachment #8874059 - Attachment is obsolete: true
Attachment #8874059 - Flags: feedback?(mdeboer)
Attachment #8874062 - Flags: feedback?(mdeboer)
Attachment #8874062 - Flags: feedback?(bugs)
(Assignee)

Comment 21

2 years ago
Created attachment 8874063 [details] [diff] [review]
cpp_xpath.p2.v3.patch
Attachment #8874063 - Flags: feedback?(mdeboer)
Attachment #8874063 - Flags: feedback?(bugs)
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"));
    }
  }
(Assignee)

Comment 23

2 years ago
Created attachment 8874290 [details] [diff] [review]
cpp_xpath.p3.v3.patch
Attachment #8874290 - Flags: feedback?(mdeboer)
Attachment #8874290 - Flags: feedback?(bugs)
(Assignee)

Comment 24

2 years ago
Created attachment 8874661 [details] [diff] [review]
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
Attachment #8874062 - Attachment is obsolete: true
Attachment #8874063 - Attachment is obsolete: true
Attachment #8874290 - Attachment is obsolete: true
Attachment #8874062 - Flags: feedback?(mdeboer)
Attachment #8874062 - Flags: feedback?(bugs)
Attachment #8874063 - Flags: feedback?(mdeboer)
Attachment #8874063 - Flags: feedback?(bugs)
Attachment #8874290 - Flags: feedback?(mdeboer)
Attachment #8874290 - Flags: feedback?(bugs)
Flags: needinfo?(bzbarsky)
Attachment #8874661 - Flags: feedback?(mdeboer)
Attachment #8874661 - Flags: feedback?(bugs)
(Assignee)

Comment 25

2 years ago
Created attachment 8874662 [details] [diff] [review]
cpp_xpath.p2.v4.patch
Attachment #8874662 - Flags: feedback?(mdeboer)
Attachment #8874662 - Flags: feedback?(bugs)
> 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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 27

2 years ago
Created attachment 8874668 [details] [diff] [review]
cpp_xpath.p1.v5.patch

Changed as suggested by Boris.
Attachment #8874661 - Attachment is obsolete: true
Attachment #8874662 - Attachment is obsolete: true
Attachment #8874661 - Flags: feedback?(mdeboer)
Attachment #8874661 - Flags: feedback?(bugs)
Attachment #8874662 - Flags: feedback?(mdeboer)
Attachment #8874662 - Flags: feedback?(bugs)
Attachment #8874668 - Flags: feedback?(mdeboer)
(Assignee)

Comment 28

2 years ago
Created attachment 8874669 [details] [diff] [review]
cpp_xpath.p2.v5.patch
Attachment #8874669 - Flags: feedback?(mdeboer)
Attachment #8874669 - Flags: feedback?(bugs)
Comment on attachment 8874668 [details] [diff] [review]
cpp_xpath.p1.v5.patch

A DOM-peer's feedback is most important here.
Attachment #8874668 - Flags: feedback?(bugs)
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!
Attachment #8874669 - Flags: feedback?(mdeboer) → feedback+
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/
Attachment #8874668 - Flags: feedback?(mdeboer) → feedback+
(Reporter)

Comment 32

2 years ago
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
Attachment #8874668 - Flags: feedback?(bugs)
(Reporter)

Comment 33

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

This needs some changes too after the patch has been fixed.
Attachment #8874669 - Flags: feedback?(bugs)
(Assignee)

Comment 34

2 years ago
Created attachment 8875607 [details] [diff] [review]
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.
Attachment #8874668 - Attachment is obsolete: true
Attachment #8874669 - Attachment is obsolete: true
Attachment #8875607 - Flags: review?(mdeboer)
Attachment #8875607 - Flags: review?(bugs)
(Assignee)

Comment 35

2 years ago
Created attachment 8875608 [details] [diff] [review]
cpp_xpath.p2.v6.patch
Attachment #8875608 - Flags: review?(mdeboer)
Attachment #8875608 - Flags: review?(bugs)
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'?
Attachment #8875607 - Flags: review?(mdeboer) → review-
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.
Attachment #8875608 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 38

2 years ago
Created attachment 8876025 [details] [diff] [review]
cpp_xpath.p1.v7.patch

Updated as Mike's requests.
Attachment #8875607 - Attachment is obsolete: true
Attachment #8875608 - Attachment is obsolete: true
Attachment #8875607 - Flags: review?(bugs)
Attachment #8875608 - Flags: review?(bugs)
Attachment #8876025 - Flags: review?(mdeboer)
Attachment #8876025 - Flags: review?(bugs)
(Assignee)

Comment 39

2 years ago
Created attachment 8876026 [details] [diff] [review]
cpp_xpath.p2.v7.patch
Attachment #8876026 - Flags: review?(mdeboer)
Attachment #8876026 - Flags: review?(bugs)
(Reporter)

Comment 40

2 years ago
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.
Attachment #8876026 - Flags: review?(bugs) → review-
(Reporter)

Comment 41

2 years ago
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.
Attachment #8876025 - Flags: review?(bugs) → review-
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?
Attachment #8876025 - Flags: review?(mdeboer) → review-
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.
Attachment #8876026 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 44

2 years ago
Created attachment 8876605 [details] [diff] [review]
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.
Attachment #8876025 - Attachment is obsolete: true
Attachment #8876026 - Attachment is obsolete: true
Attachment #8876605 - Flags: review?(mdeboer)
Attachment #8876605 - Flags: review?(bugs)
(Assignee)

Comment 45

2 years ago
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 ",'. 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.
Attachment #8876607 - Flags: review?(mdeboer)
Attachment #8876607 - Flags: review?(bugs)
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!
Attachment #8876605 - Flags: review?(mdeboer) → review+
(Reporter)

Comment 47

2 years ago
(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().
(Reporter)

Comment 48

2 years ago
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
Attachment #8876607 - Flags: review?(bugs) → review+
(Reporter)

Comment 49

2 years ago
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.
Attachment #8876605 - Flags: review?(bugs) → review-
(Assignee)

Comment 50

2 years ago
Created attachment 8877843 [details] [diff] [review]
cpp_xpath.p1.v9.patch

Changed as Olli's suggestions.
Attachment #8876605 - Attachment is obsolete: true
Attachment #8877843 - Flags: review?(bugs)
(Assignee)

Comment 51

2 years ago
(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',"''",'').
Attachment #8876607 - Flags: review?(mdeboer) → review+
(Reporter)

Comment 52

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

Have you run this through tryserver?
Attachment #8877843 - Flags: review?(bugs) → review+
(Assignee)

Comment 53

2 years ago
(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

Updated

2 years ago
Blocks: 1373672

Comment 54

2 years ago
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!

Updated

2 years ago
No longer blocks: 1373672

Comment 55

2 years ago
Sorry for the noise, looks like this bug may not help with the benchmark after all.
(Assignee)

Comment 56

2 years ago
Created attachment 8879016 [details] [diff] [review]
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
Attachment #8877843 - Attachment is obsolete: true
Attachment #8879016 - Flags: review?(mdeboer)
Attachment #8879016 - Flags: review?(mdeboer) → review+
Requesting checkin for both patches (p1 and p2, in that order); both are 'r=smaug,mikedeboer'. Thank you!
Keywords: checkin-needed

Comment 58

2 years ago
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
Keywords: checkin-needed
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
Flags: needinfo?(mdeboer)
Beekill, sorry this patch had to be backed out, see the previous comment. Please fix the issue and submit an updated patch.
Flags: needinfo?(nnn_bikiu0707)
(Assignee)

Comment 61

2 years ago
Created attachment 8879879 [details] [diff] [review]
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
Flags: needinfo?(nnn_bikiu0707) → needinfo?(aryx.bugmail)
Flags: needinfo?(mdeboer)
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?
Flags: needinfo?(aryx.bugmail) → needinfo?(bugs)
(Reporter)

Comment 63

2 years ago
So isn't comment 59 just about some missing #includes? They may not show up on all the built setup because of unified building.
Flags: needinfo?(bugs)
Yup, sure seems like it. Quan, can you take a look?
Flags: needinfo?(nnn_bikiu0707)
(Reporter)

Comment 65

2 years ago
was the issue coming from bug 1373536 after all?
(Reporter)

Comment 66

2 years ago
or perhaps that triggered the same issue. Look at the last patch there.
(Assignee)

Comment 67

2 years ago
Created attachment 8880251 [details] [diff] [review]
cpp_xpath.p3.v10.patch

Added some includes as Olli suggested.
Flags: needinfo?(nnn_bikiu0707)
Attachment #8880251 - Flags: review?(bugs)
(Reporter)

Comment 68

2 years ago
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.
Attachment #8880251 - Flags: review?(bugs)
(Assignee)

Comment 69

2 years ago
(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.
Flags: needinfo?(bugs)
(Reporter)

Comment 70

2 years ago
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 71

2 years ago
Created attachment 8882998 [details] [diff] [review]
cpp_xpath.p1.v10.patch
Attachment #8876607 - Attachment is obsolete: true
Attachment #8879016 - Attachment is obsolete: true
Attachment #8879879 - Attachment is obsolete: true
Attachment #8880251 - Attachment is obsolete: true
(Assignee)

Comment 72

2 years ago
Created attachment 8882999 [details] [diff] [review]
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
(Assignee)

Comment 73

2 years ago
Created attachment 8883000 [details] [diff] [review]
cpp_xpath.p1.v10.patch
Attachment #8882998 - Attachment is obsolete: true
Re-requesting checkin for both patches (p1 and p2, in that order). Thanks!
Keywords: checkin-needed

Comment 75

2 years ago
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
Keywords: checkin-needed
(Assignee)

Comment 78

2 years ago
Created attachment 8883234 [details] [diff] [review]
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
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883234 - Flags: review?(bugs)
(Reporter)

Updated

2 years ago
Attachment #8883234 - Flags: review?(bugs) → review+
Re-requesting checkin for all three patches (p1, p2 and p3, in that order). Thanks!
Keywords: 64bit, checkin-needed
Keywords: 64bit

Comment 80

2 years ago
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
Keywords: checkin-needed

Comment 81

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72ef9aacfb91
https://hg.mozilla.org/mozilla-central/rev/d1ab638d4180
https://hg.mozilla.org/mozilla-central/rev/098f29ddc693
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.