Closed Bug 1445080 Opened 6 years ago Closed 6 years ago

remote-browser depends on consumer to setup correctly

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Version:
58 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 + verified
firefox61 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

Bug 1445080 - fix handling of remote web progress for non-tab browsers,
59 bytes, text/x-review-board-request
Gijs
: review+
mconley
: review+
jcristau
: approval-mozilla-beta+
Details 
In a followup to bug 1443749, I discovered that a remote browser (in sidebar) did not have currentURI set correctly after a document was loaded.  This likely also affects browser and page actions, though we don't hit this specific issue as we are not touching currentURI.

By adding: "browser.webProgress;" after inserting the browser into the document, RemoteWebProgress is created and currentURI is set correctly.  We shouldn't depend on the consumer of the remote browser doing this.
This is going to need uplift to 60 in order to fix issues with sidebar extensions, such as Notes and Tab Split.
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox61: --- → affected
tracking-firefox60: --- → ?
Comment on attachment 8958277 [details]
Bug 1445080 - fix handling of remote web progress for non-tab browsers,

https://reviewboard.mozilla.org/r/227210/#review233138

Good news, even in the bright light of morning, this looks sensible to me, assuming Mike agrees. :-)

::: toolkit/content/widgets/remote-browser.xml:53
(Diff revision 1)
>                  readonly="true"/>
>  
>        <field name="_remoteWebProgress">null</field>
>  
>        <property name="webProgress" readonly="true">
>        	<getter>

Nit: can you fix the tabs while you're here?
Attachment #8958277 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8958277 [details]
Bug 1445080 - fix handling of remote web progress for non-tab browsers,

https://reviewboard.mozilla.org/r/227210/#review233282

Yeah, this looks reasonable. Thanks, mixedpuppy!

::: commit-message-d6957:1
(Diff revision 2)
> +Bug 1445080 fix handling of remote web progress for non-tab browsers, r?Gijs,mconley

Nit: I think normally a - or a : goes between the bub number and the commit message. Maybe reformat as:

Bug 1445080 - Fix handling of remote web progress for non-tab browsers. r?Gijs,mconley
Attachment #8958277 - Flags: review?(mconley) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c204ccf3a7c
fix handling of remote web progress for non-tab browsers, r=Gijs,mconley
https://hg.mozilla.org/mozilla-central/rev/4c204ccf3a7c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8958277 [details]
Bug 1445080 - fix handling of remote web progress for non-tab browsers,

Approval Request Comment
[Feature/Bug causing the regression]: remote-browser
[User impact if declined]: non-tab browser used in sidebar reloads too often
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Verifying fixes that resulted in these bugs would be good even though there is a test.  QA for notes and tab-split has several github issues with STR
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Risk is moderate.
[Why is the change risky/not risky?]:  The change is simple, however it affects all remote browser elements
[String changes made/needed]: none
Attachment #8958277 - Flags: approval-mozilla-beta?
Flags: qe-verify+
This seemed to fix the problems that Notes had, I am gonna go through the bugs we have in Github and SoftVision will do another run at this. Thank you!
Comment on attachment 8958277 [details]
Bug 1445080 - fix handling of remote web progress for non-tab browsers,

fix issues with sidebar extensions, beta60+
Attachment #8958277 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We've retested the Firefox Notes and Tab Split issues filed from the corresponding GitHub repositories and the issues are no longer reproducible on latest Nightly build 61.0a1 (Build ID: 20180321220044) and latest Beta 60.0b5 (Build ID: 20180319175655), on Windows 10 x64, Mac 10.13.3, Ubuntu 14.04 x64 and Arch Linux 4.12.

From the 2 webextensions side, this change fixed the problems. However I'm not sure how can we test if the currentURI is set correctly, and confirm that everything fixed in this issue is as it should be.  
Shane, can you please provide a minimal test case and some information regarding how we can QA this?
Flags: needinfo?(mixedpuppy)
An automated test was added for currentURI.  The only manual verification necessary is that the extensions were fixed.
Flags: needinfo?(mixedpuppy)
All right, we are good then. Thank you!
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
Based on previous comments, this bug was verified on Nightly 60.0a1 and Beta 60.0b5. Removing the qe-verify+ flag.
Flags: qe-verify+
Assignee: nobody → mixedpuppy
Blocks: 1581300
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: