764572 - add "Open URL" option to net panel items' context menu

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Luke Crouch [:groovecoder]]

When I see this: 

[15:58:52.106] GET https://developer-dev.allizom.org/en-US/docs/ckeditor_config.js?t=B8DJ5M3 [HTTP/1.1 200 OK 68ms]

item in my Net panel, I want to easily right-click to open URL in a new tab.

Comment 1

[Josh Matthews [:jdm]]

Link to relevant source?

Comment 2

[Luke Crouch [:groovecoder]]

I hope that question isn't for me. :) I have no idea.

Comment 3

[Sankha Narayan Guria [:sankha]]

If I am on the right track, does this bug mean that there should be an "Open URL" in the right click context menu for links in the Web Console to open the link in a new tab?

Then can anybody suggest the relevant sources, so that I can take a look into it?

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to Sankha Narayan Guria from comment #3)
> If I am on the right track, does this bug mean that there should be an "Open
> URL" in the right click context menu for links in the Web Console to open
> the link in a new tab?
> 
> Then can anybody suggest the relevant sources, so that I can take a look
> into it?

This should be the method (HUD_createConsoleMenu()) which create the context menu
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm#1710

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch1

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch part2

Comment 7

[Mihai Sucan [:msucan]]

I've been assigned as a mentor for this bug, but no CC - I didn't know.... I just saw now Ohzeki asked questions which I didn't reply to. I apologize for that!

Rob, Ohzeki asked for your review. Do you want to be his mentor? If you feel you're too busy, I can review the code myself.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mihai Sucan [:msucan] from comment #7)
> I've been assigned as a mentor for this bug, but no CC - I didn't know.... I
> just saw now Ohzeki asked questions which I didn't reply to. I apologize for
> that!
> 
> Rob, Ohzeki asked for your review. Do you want to be his mentor? If you feel
> you're too busy, I can review the code myself.

Mihai, I'm sorry that I didn't ask for your review. I could not know your mail address. So I ask the review of Rob who I saw many scenes of reviewing about devtools. Never mind.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #8)
> Mihai, I'm sorry that I didn't ask for your review. I could not know your
> mail address. So I ask the review of Rob who I saw many scenes of reviewing
> about devtools. Never mind.

Don't worry. Indeed, Rob has been reviewing a ton of Web Console patches I've been working on. We are both happy to see contributors submitting patches! ;)

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 646846 [details] [diff] [review]
patch part2

Review of attachment 646846 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is broken by checking in http://hg.mozilla.org/mozilla-central/rev/637461e0e2ad.

I'm working to make the new patch.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Mihai, please help Ohzeki along with this.

Ohzeki, no worries about asking me or Mihai for review. As he says, we're both happy to help.

Also, as a point of order, you don't have to R- your own patches. Just clear out the ? and the name from the form to cancel the review.

Thanks for working on this. :)

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 646846 [details] [diff] [review]
patch part2

WIP patches to be completed in another follow-up. Cancelling review flags.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1 rev.2

Rebased on latest-mozilla-central.

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2 rev.2

Rebased on latest-mozilla-central.

Comment 15

[Mihai Sucan [:msucan]]

Ohzeki: thanks for the updated patches!

Review comments:

- do you really need to change createMessageNode()? You can add request.url to the node in logNetActivity() on the node returned by createMessageNode(). I'd like us to not grow the complexity ofg that method.

- in openSelectedItemInTab() you can use outputNode.selectedItem.url. You don't need to loop over the children.

- what happens when you right-click on a message which doesn't have a URL?

For the next update, please merge the two patches into one, to make it easier to review the code. Thanks!

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 647486 [details] [diff] [review]
part1 rev.2

Canceling review request - waiting for further patch changes.

Comment 17

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mihai Sucan [:msucan] from comment #15) 
> - do you really need to change createMessageNode()? You can add request.url
> to the node in logNetActivity() on the node returned by createMessageNode().
> I'd like us to not grow the complexity ofg that method.
>
I thought that it should be set in createMessageNode() if we try to set a data. Because the way to set a data to node may be changed in the future. (then, we uses WeakMap & getter implementation). However, we might need to refacor createMessageNode(). its method has too many parameters. It is bad design...


> - in openSelectedItemInTab() you can use outputNode.selectedItem.url. You
> don't need to loop over the children.
>
I thought that user may think opening the multiple urls If user selected multiple items.
Do not we need to consider it which is rare case?


> - what happens when you right-click on a message which doesn't have a URL?
>
This is my miss. I'll implement support its case. I think that showing items only when the selected items have an url. But if user selects multiple message items, |CommandController.isCommandEnabled()| may be heavy… This is related to above 2nd thing.

What do you think?

Comment 18

[Mihai Sucan [:msucan]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #17)
> (In reply to Mihai Sucan [:msucan] from comment #15) 
> > - do you really need to change createMessageNode()? You can add request.url
> > to the node in logNetActivity() on the node returned by createMessageNode().
> > I'd like us to not grow the complexity ofg that method.
> >
> I thought that it should be set in createMessageNode() if we try to set a
> data. Because the way to set a data to node may be changed in the future.
> (then, we uses WeakMap & getter implementation). However, we might need to
> refacor createMessageNode(). its method has too many parameters. It is bad
> design...

True. We are in the process of rewriting how output works. See bug 778766. 


> > - in openSelectedItemInTab() you can use outputNode.selectedItem.url. You
> > don't need to loop over the children.
> >
> I thought that user may think opening the multiple urls If user selected
> multiple items.
> Do not we need to consider it which is rare case?

I don't think we want to open all urls at once. It might cause unexpected behavior. Also, after bug 778766 selection behavior will change.

When I right-click on one item I expect "open URL' will only open the link I point to.


> > - what happens when you right-click on a message which doesn't have a URL?
> >
> This is my miss. I'll implement support its case. I think that showing items
> only when the selected items have an url. But if user selects multiple
> message items, |CommandController.isCommandEnabled()| may be heavy… This is
> related to above 2nd thing.
> 
> What do you think?

isCommandEnabled can perform well if you only check if there's one selected item. Based on that you can have the menuitem enabled/disabled.

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev.3

I have updated the patch.
-Merge patches.
-Show the command only if an item (activity node) has "url" property.
-Open only 1 url.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 647656 [details] [diff] [review]
patch rev.3

Review of attachment 647656 [details] [diff] [review]:
-----------------------------------------------------------------

This is now better. Thanks for your quick update!

This is now close to be done, but please address the comments below.

Also, do note that the menuitem doesn't show as disabled when I right-click different message types. Can you look into why this doesn't work?

::: browser/devtools/webconsole/webconsole.js
@@ +1157,5 @@
>  
>      let clipboardText = request.method + " " + request.url;
>  
> +    let metadata = {
> +      url: request.url,

I keep my request: please do not add a new metadata argument to createMessageNode(). Simply do messageNode.url = request.url, after the createMessageNode() call. Thanks!

@@ +2106,5 @@
> +   */
> +  openSelectedItemInTab: function HS_openSelectedItemInTab () {
> +    let item = this.outputNode.selectedItem;
> +
> +    if (!item.url) {

What if there's no selectedItem? selectedItem can be null.

@@ +3273,5 @@
>          return true;
> +      case "consoleCmd_openURL":
> +        // Only enable "open url" if node is Net Activity.
> +        let selectedItem = this.owner.outputNode.selectedItem;
> +        return (selectedItem.url) ? true : false;

Same concern here.

Comment 21

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev.4

(In reply to Mihai Sucan [:msucan] from comment #20)
> Also, do note that the menuitem doesn't show as disabled when I right-click
> different message types. Can you look into why this doesn't work?

I fixed it on the patch rev. 4.
It needs to use xul commandupdater attribute:
https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Updating_Commands

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 648958 [details] [diff] [review]
patch rev.4

Review of attachment 648958 [details] [diff] [review]:
-----------------------------------------------------------------

r+ - I like the patch! Thanks for your work!

Can you please also write a test? We need a test before we can land the patch.

Learn about browser tests here: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests
The Web Console tests are in browser/devtools/webconsole/test. Please let me know if you have questions. I suggest you copy an existing test and modify it to test the new web console functionality.

::: browser/devtools/webconsole/webconsole.js
@@ +1161,5 @@
>      let messageNode = this.createMessageNode(CATEGORY_NETWORK, severity,
>                                               msgNode, null, null, clipboardText);
>  
>      messageNode._connectionId = entry.connection;
> +    messageNode.url = url;

instead of doing let url = request.url and all the other changes in this method, you could simply do only messageNode.url = request.url, to keep the patch smaller.

@@ +2089,5 @@
>  
>    /**
> +   * Open the selected item's url in new tabs.
> +   */
> +  openSelectedItemInTab: function HS_openSelectedItemInTab () {

Please change HS_ to WCF_. Also, no space after the function name. Put the curly brace on a new line, like the rest of the code.

@@ +3238,5 @@
> +  /**
> +   * Open urls of the currently-selected entries in the Web Console output
> +   * in a new tab.
> +   */
> +  openURL: function CommandController_openURL ()

nit: no space after the function name.

@@ +3259,5 @@
>          return true;
> +      case "consoleCmd_openURL":
> +        // Only enable "open url" if node is Net Activity.
> +        let selectedItem = this.owner.outputNode.selectedItem;
> +        return (selectedItem && selectedItem.url) ? true : false;

return selectedItem && selectedItem.url;

@@ +3285,5 @@
>    return gSequenceId.n++;
>  }
>  gSequenceId.n = 0;
>  
> +function goUpdateConsoleCommands () {

nits: space after function name and curly brace on a new line.

Comment 23

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mihai Sucan [:msucan] from comment #22)

Thank you for your speedy review!

 
> Can you please also write a test? We need a test before we can land the
> patch.

I'm trying to write the test in a new file.
If I get a question, I may ask you about it. Then, Please give me a nice advice.


> @@ +3259,5 @@
> >          return true;
> > +      case "consoleCmd_openURL":
> > +        // Only enable "open url" if node is Net Activity.
> > +        let selectedItem = this.owner.outputNode.selectedItem;
> > +        return (selectedItem && selectedItem.url) ? true : false;
> 
> return selectedItem && selectedItem.url;
>

I think that, the conditional operator often confuses the code, so its conditional part should be divided with round bracket.

Comment 24

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev.5

Add the test.

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 653294 [details] [diff] [review]
patch rev.5

Review of attachment 653294 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the test. It works well! I have some minor comments below...

::: browser/devtools/webconsole/test/browser_webconsole_bug_764572_output_open_url.js
@@ +4,5 @@
> +
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console.html"
> +const COMMAND_NAME = "consoleCmd_openURL";
> +
> +let HUD = null, outputNode = null;

Please add a comment to this test file that shortly explain what the test is for.

@@ +24,5 @@
> +
> +function testOnNotNetActivity() {
> +  HUD.jsterm.clearOutput();
> +
> +  // See bugs 574036, 586386 and 587617.

This looks to be a comment copied from another test. Please remove this.

@@ +38,5 @@
> +    successFn: function () {
> +      outputNode.selectedIndex = 0;
> +      outputNode.focus();
> +
> +      outputNode.getItemAtIndex(0);

This approach is prone to failures because other messages might be added later on, breaking ordering. Please use outputNode.querySelector(".webconsole-msg-log"); (or something like this - you might want to use the DOM Inspector addon [1] to check for the correct class name)

[1] https://addons.mozilla.org/en-us/firefox/addon/dom-inspector-6622/

@@ +65,5 @@
> +    validatorFn: function () {
> +      outputNode.selectedIndex = 0;
> +      outputNode.focus();
> +
> +      let item = outputNode.getItemAtIndex(0);

Same as above. Here the class name is probably .webconsole-msg-network or .webconsole-msg-networkinfo (use the DOM Inspector to determine the correct class name).

@@ +101,5 @@
> +    validatorFn: function()
> +    {
> +      // wait to complete initializing the opend tab.
> +      let url = aTab.linkedBrowser.currentURI.spec;
> +      return url !== "about:blank";

Why not directly wait for TEST_URI here?

::: browser/devtools/webconsole/webconsole.js
@@ +3391,5 @@
>          return true;
> +      case "consoleCmd_openURL":
> +        // Only enable "open url" if node is Net Activity.
> +        let selectedItem = this.owner.outputNode.selectedItem;
> +        return (selectedItem && selectedItem.url) ? true : false;

Sorry, but I'd like this changed. This is more confusing than a simple return foo && foo.bar.

Comment 26

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch rev.6

Thank you for speedy review!
I update the patch. Please review again.

(In reply to Mihai Sucan [:msucan] from comment #25)
> @@ +101,5 @@
> > +    validatorFn: function()
> > +    {
> > +      // wait to complete initializing the opend tab.
> > +      let url = aTab.linkedBrowser.currentURI.spec;
> > +      return url !== "about:blank";
> 
> Why not directly wait for TEST_URI here?

I thought that it is better that waiting to complete initializing the tab.
However, this is meaningless & wordy as you pointed.
I change it at this revision :)

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 653656 [details] [diff] [review]
patch rev.6

Review of attachment 653656 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your update. I was about to r+ this patch, but after playing some time with this new feature I came to the conclusion it's weird to show a disabled menu item for all of the non-network messages. Can you please hide the menuitem when the user right-clicks on a non-network messages? To do this you can add an event listener fior the context menupopup, listen for the popupshowing event. In the event handler check which kind of message the user right clicked on, then hide/show the open URL menu item.

::: browser/devtools/webconsole/test/browser_webconsole_bug_764572_output_open_url.js
@@ +4,5 @@
> +
> +// This html is for loading to test
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console.html"
> +// test command name
> +const COMMAND_NAME = "consoleCmd_openURL";

When I asked for a comment to describe the test I did not mean comments for these two constants.

I meant a general comment: "This is a test for the Open URL context menu item that is shown for network requests".

::: browser/devtools/webconsole/webconsole.xul
@@ +31,5 @@
>    <popupset id="mainPopupSet">
>      <menupopup id="output-contextmenu">
>        <menuitem id="saveBodies" type="checkbox" label="&saveBodies.label;"
>                  accesskey="&saveBodies.accesskey;"/>
> +      <menuitem id="openURL" label="&openURL.label;"

Please change to id="menu_openURL"

Comment 28

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mihai Sucan [:msucan] from comment #27)
> Thanks for your update. I was about to r+ this patch, but after playing some
> time with this new feature I came to the conclusion it's weird to show a
> disabled menu item for all of the non-network messages. Can you please hide
> the menuitem when the user right-clicks on a non-network messages? To do
> this you can add an event listener fior the context menupopup, listen for
> the popupshowing event. In the event handler check which kind of message the
> user right clicked on, then hide/show the open URL menu item.

I agree your idea. It's good.
However, I seem that it is better that implementing your idea may be filed as other bug & we split into an other patch. Although it depending on scale, I would like to implement the hide/show the menuitem function for a general purpose.
If you agree a my proposal, I'll file the new bug & start planning to implementation it.


> When I asked for a comment to describe the test I did not mean comments for
> these two constants.
> 
> I meant a general comment: "This is a test for the Open URL context menu
> item that is shown for network requests".

Sorry. I had misread. I fix it at the next patch.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #28)
> (In reply to Mihai Sucan [:msucan] from comment #27)
> > Thanks for your update. I was about to r+ this patch, but after playing some
> > time with this new feature I came to the conclusion it's weird to show a
> > disabled menu item for all of the non-network messages. Can you please hide
> > the menuitem when the user right-clicks on a non-network messages? To do
> > this you can add an event listener fior the context menupopup, listen for
> > the popupshowing event. In the event handler check which kind of message the
> > user right clicked on, then hide/show the open URL menu item.
> 
> I agree your idea. It's good.
> However, I seem that it is better that implementing your idea may be filed
> as other bug & we split into an other patch. Although it depending on scale,
> I would like to implement the hide/show the menuitem function for a general
> purpose.
> If you agree a my proposal, I'll file the new bug & start planning to
> implementation it.

Unfortunately, the current behavior is not something I would like us to land. Showing an "Open URL in a new tab" menu item that is disabled for the majority of messages in the Web Console is not too nice for users.

If you feel it's more comfortable, please use a mercurial patch queue, and make a patch on top of this patch you have here. Like you suggest - a split into another patch, but we keep it in this bug. When the second patch is ready, we can land both of them together.

> > When I asked for a comment to describe the test I did not mean comments for
> > these two constants.
> > 
> > I meant a general comment: "This is a test for the Open URL context menu
> > item that is shown for network requests".
> 
> Sorry. I had misread. I fix it at the next patch.

Thanks!

Comment 30

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Mihai:
OK. I try to make a new patch for web-console contextmenu.

Comment 31

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1:Implement "Open URL". rev.7

Rebased on latest mozilla-central.

Comment 32

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2: Implement to handle ctx-menu

I consult Places's contextmenu to implement this.

Comment 33

[Mihai Sucan [:msucan]]

Comment on attachment 659130 [details] [diff] [review]
part1:Implement "Open URL". rev.7

This is fine. As long as you only rebase a patch, a re-review is not needed.

Thanks for your updates!

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 659131 [details] [diff] [review]
part2: Implement to handle ctx-menu

Review of attachment 659131 [details] [diff] [review]:
-----------------------------------------------------------------

This works well and I like it. Thank you!

Please update the test file you added in part 1, to check if the menu item is shown/hidden as needed.

Minor comments below...

::: browser/devtools/webconsole/webconsole.js
@@ +3507,5 @@
> +
> +/*
> + * ConsoleCtxMenu: This handle to show/hide a context menu item.
> + */
> +let ConsoleCtxMenu = {

ConsoleContextMenu

@@ +3509,5 @@
> + * ConsoleCtxMenu: This handle to show/hide a context menu item.
> + */
> +let ConsoleCtxMenu = {
> +
> +  /*

Stray empty line?

@@ +3517,5 @@
> +   */
> +  build: function (aEvent)
> +  {
> +    let popup = aEvent.target;
> +    if (popup.id !== CONTEXTMENU_ID) {

Is this needed? Will the event fire at different targets?

@@ +3521,5 @@
> +    if (popup.id !== CONTEXTMENU_ID) {
> +      return;
> +    }
> +
> +    let view = document.getElementById(CONSOLE_VIEW_ID);

You could just do document.querySelector(".hud-output-node"). No need to add an element ID.

@@ +3586,5 @@
> +  {
> +    let shouldHide = true;
> +
> +    let selectionType = aMenuItem.getAttribute("selectiontype");
> +    if (selectionType && !aMetadata.selectionType.has(selectionType)) {

metadata.selectionType is a string, so this will break.

@@ +3587,5 @@
> +    let shouldHide = true;
> +
> +    let selectionType = aMenuItem.getAttribute("selectiontype");
> +    if (selectionType && !aMetadata.selectionType.has(selectionType)) {
> +      return shouldHide;

Just return true.

@@ +3592,5 @@
> +    }
> +
> +    let selection = aMenuItem.getAttribute("selection");
> +    if (!selection) {
> +      return shouldHide = false;

This looks weird. Better just return false.

@@ +3606,5 @@
> +    }
> +
> +    return shouldHide;
> +  },
> +

Stray empty line.

Comment 35

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1:Implement "Open URL". rev.7.1

Rebased.

Comment 36

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2: Implement to handle ctx-menu rev2

Rebase & add test!

(In reply to Mihai Sucan [:msucan] from comment #34) 
> @@ +3517,5 @@
> > +   */
> > +  build: function (aEvent)
> > +  {
> > +    let popup = aEvent.target;
> > +    if (popup.id !== CONTEXTMENU_ID) {
> 
> Is this needed? Will the event fire at different targets?

Yes, this is needed.
If we'll have added sub-menu to the context menu in the future, DOM event will propagate from sub-menu when opening the sub-menu. This part cut off such case.


> @@ +3521,5 @@
> > +    if (popup.id !== CONTEXTMENU_ID) {
> > +      return;
> > +    }
> > +
> > +    let view = document.getElementById(CONSOLE_VIEW_ID);
> 
> You could just do document.querySelector(".hud-output-node"). No need to add
> an element ID.

I think that this case should use id. Because looking up an element from id is speedy. In this case, ConsoleContextMenu.build() may be called many times & it go directly to user feeling. So we should document.getElementById().

Comment 37

[Mihai Sucan [:msucan]]

Comment on attachment 661158 [details] [diff] [review]
part2: Implement to handle ctx-menu rev2

Review of attachment 661158 [details] [diff] [review]:
-----------------------------------------------------------------

Giving r+ with the comments below addressed. Also, please move the new functions from head.js into your new test file - I do not feel they should be in head.js yet. We will put them in head.js later, if we need them in more tests. Thank you for your patience!

::: browser/devtools/webconsole/test/browser_webconsole_bug_764572_output_open_url.js
@@ +10,2 @@
>  
> +let HUD = null, outputNode = null, contextMenu = null;

The contextMenu global and the CONTEXT_MENU constant have the same name and that's very confusing.

Can you rename the constant to CONTEXT_MENU_ID?

@@ +66,5 @@
> +  waitForOpenContextMenu(contextMenu, {
> +    target: target,
> +    successFn: function () {
> +      let isHidden = contextMenu.querySelector(CONTEXT_MENU).hidden;
> +      ok(isHidden, CONTEXT_MENU+"should be hidden.");

nit: spaces before and after +.

::: browser/devtools/webconsole/webconsole.js
@@ +3541,5 @@
> +   * Handle to show/hide context menu item.
> +   *
> +   * @param nsIDOMEvent aEvent
> +   */
> +  build: function (aEvent)

Please add function names to all of the new methods. Example: CCM_build... and so on.

Comment 38

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2: Implement to handle ctx-menu rev3

Mihai, Thank you for your speedy review!
I hope this review request will be last & we can land these patches ;)

And I pushed to try-server:
https://tbpl.mozilla.org/?tree=Try&rev=b87a91e2a2b5

Comment 39

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Re-push
https://tbpl.mozilla.org/?tree=Try&rev=24f1671c2273

Comment 40

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Umm...
On my local build, Tests added with this bug pass without any error...

Comment 41

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part2: Implement to handle ctx-menu rev3.1

I fix the mistake.

This pass the test as green:
https://tbpl.mozilla.org/?tree=Try&rev=04901708c33c

Please review this.

Comment 42

[Mihai Sucan [:msucan]]

Comment on attachment 661494 [details] [diff] [review]
part2: Implement to handle ctx-menu rev3.1

Thank you!

Comment 43

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] folded patch

Landed:
https://hg.mozilla.org/integration/fx-team/rev/7907bdc1be3c

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7907bdc1be3c