1241892 - "Page Info" context menu items don't work in sidebar (was intentionally broken in bug 1238180)

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) Create a bookmark
2) In the bookmarks manager, select the bookmark, and click the down arrow under "Tags" to expand your options.
3) Check "Load this bookmark in the sidebar"
4) Open the bookmark
5) The bookmarked page should now load in the sidebar browser.
6) Right-click in the sidebar browser, and choose "View Page Info" or "Inspect Element" (or "View Image Info" if hovering an image, or any of the items under "This Frame" if hovering over an iframe). Also try "View Page Source".

These items don't work.

Comment 1

[arni2033]

The title is too general, because almost all menu items were regressed one by one,
and also this is duplicate of bug 1241147

Comment 3

[Justin Dolske [:Dolske]]

mconley: Since bug 1238180 regressed this, do you know what's involved here to fix this?

I know some people don't like the sidebar, but since it's still supported, we should probably not have busted menuitems.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Most of the breakage is because a lot of the context menu stuff (and operations) was converted to work with e10s assuming that gBrowser was available in the window scope, which is not true in the web panel.

I think it shouldn't be too bad to patch some of this - incoming...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1241892 - Make Page Info work for sidebar browser.

The modifications that were made in bug 1238180 assumed that
gBrowser.selectedBrowser from the opening window context would
be defined. This is true in normal browser tabs, but not so much
in the sidebar browser. This patch makes it so that callers can
supply a browser to override the assumption.

Review commit: https://reviewboard.mozilla.org/r/62154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62154/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1241892 - Make View Source work in the sidebar browser.

The sidebar browser is always non-remote, so when we open a new tab
to open view source for it, we should ensure that that tab is also
non-remote.

Review commit: https://reviewboard.mozilla.org/r/62156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62156/

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8767752 [details]
Bug 1241892 - Make Page Info work for sidebar browser.

https://reviewboard.mozilla.org/r/62154/#review59116

Looks reasonable.

One issue I observed is that when opening Page Info on the page loaded in the sidebar (eg. on https://www.mozilla.org/en-US/ ), a few seconds later I get this JS error:
A coding exception was thrown and uncaught in a Task.

Full message: ReferenceError: setTimeout is not defined
Full stack: PageInfoListener.processFrames/<@chrome://browser/content/content.js:1133:40
PageInfoListener.processFrames@chrome://browser/content/content.js:1133:17
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
Task_spawn@resource://gre/modules/Task.jsm:168:12
PageInfoListener.getMediaInfo@chrome://browser/content/content.js:1099:5
PageInfoListener.receiveMessage@chrome://browser/content/content.js:1000:5

I don't know why setTimeout isn't defined for the sidebar's content script.

Something else I observed is that the top part of the context menu (back, forward, stop/reload, bookmark) isn't working on the sidebar.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8767753 [details]
Bug 1241892 - Make View Source work in the sidebar browser.

https://reviewboard.mozilla.org/r/62156/#review59238

Looks resonable to me, thanks!

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8767752 [details]
> Bug 1241892 - Make Page Info work for sidebar browser.
> 
> https://reviewboard.mozilla.org/r/62154/#review59116
> 
> Looks reasonable.
> 
> One issue I observed is that when opening Page Info on the page loaded in
> the sidebar (eg. on https://www.mozilla.org/en-US/ ), a few seconds later I
> get this JS error:
> A coding exception was thrown and uncaught in a Task.
> 
> Full message: ReferenceError: setTimeout is not defined
> Full stack:
> PageInfoListener.processFrames/<@chrome://browser/content/content.js:1133:40
> PageInfoListener.processFrames@chrome://browser/content/content.js:1133:17
> TaskImpl_run@resource://gre/modules/Task.jsm:319:40
> TaskImpl@resource://gre/modules/Task.jsm:280:3
> createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
> Task_spawn@resource://gre/modules/Task.jsm:168:12
> PageInfoListener.getMediaInfo@chrome://browser/content/content.js:1099:5
> PageInfoListener.receiveMessage@chrome://browser/content/content.js:1000:5
> 
> I don't know why setTimeout isn't defined for the sidebar's content script.
> 
> Something else I observed is that the top part of the context menu (back,
> forward, stop/reload, bookmark) isn't working on the sidebar.

Thanks - I'll file follow-ups for those.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0d26b8d20ab
Make Page Info work for sidebar browser. r=florian
https://hg.mozilla.org/integration/autoland/rev/f1d3824a31ae
Make View Source work in the sidebar browser. r=jryans

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0d26b8d20ab
https://hg.mozilla.org/mozilla-central/rev/f1d3824a31ae

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Should we consider backporting this?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

We could. Shipping broken menuitems sure doesn't look great. I'll see what relman wants to do.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8767752 [details]
Bug 1241892 - Make Page Info work for sidebar browser.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1238180.

[User impact if declined]:

Users who use the sidebar browser (a pretty hidden feature, but it's used by at least some of our user population) will find that some context menu options don't work at all for the sidebar browser. Specifically, Page Info and View Source.

This will affect users regardless of whether or not they have e10s enabled or not.

[Describe test coverage new/current, TreeHerder]:

There's unfortunately not a huge amount of test coverage for the sidebar browser context menu. I do know that we have Page Info and View Source tests, and that those continue to pass, so we haven't regressed the normal uses of those features.

[Risks and why]: 

I think this is pretty low risk.

[String/UUID change made/needed]:

None.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8767753 [details]
Bug 1241892 - Make View Source work in the sidebar browser.

Same as above.

Comment 16

[Gerry Chang [:gchang]]

Hi Mike,
If we let this ride the train on 49/50 only and won't fix in 48 beta, will this a big impact for user given this issue has been there from 45?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Gerry Chang [:gchang] from comment #16)
> Hi Mike,
> If we let this ride the train on 49/50 only and won't fix in 48 beta, will
> this a big impact for user given this issue has been there from 45?

No, I don't think so. If you want to hold this back from 48, I think that's fine - busted menu items suck, but the feature is pretty heavily buried, and (as you say), we've been shipping it this way since 45, for better or worse.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8767752 [details]
Bug 1241892 - Make Page Info work for sidebar browser.

Let's let it ride the train on 49/50 and get extra bake time.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8767753 [details]
Bug 1241892 - Make View Source work in the sidebar browser.

Per comment #18, let's let it ride the train on 49/50.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

I assume comment 19 meant to mark aurora instead of beta approval?

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8767753 [details]
Bug 1241892 - Make View Source work in the sidebar browser.

Mike, you are right, thanks for catching this.

Comment 22

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/120008d520c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/392198ab8032

Comment 23

[Md. Ehsanul Hassan]

I have reproduced this bug with Firefox Nightly 46.0a1 (Build ID:20160122030244) on 
Windows 10, 64-bit.

Verified as fixed with Firefox Developer edition 49.0a2 (Build ID: 20160723004004)
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0