tabs.update should be able to do a replace load

RESOLVED FIXED in Firefox 57
(NeedInfo from)

Status

()

Toolkit
WebExtensions: General
P5
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: bz, Assigned: bsilverberg, Mentored, NeedInfo)

Tracking

(Blocks: 1 bug, {dev-doc-complete, good-first-bug})

Trunk
mozilla57
dev-doc-complete, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [design-decision-approved][tabs])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Right now tabs.update has no way to specify that the load should replace the current history entry for the targeted tab.  Maybe we should have a way to do that.
This would approximately equivalent to `window.location.replace()`, so it should be pretty noncontroversial.

It should be fairly easy to implement. We'd need to add an extra property to the update properties object ("loadReplace"?):

http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/components/extensions/schemas/tabs.json#709-752

And then switch from loadURI to loadURIWithFlags here, and pass the Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY flag when that property is true:

http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/components/extensions/ext-tabs.js#441

And probably add a test:

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_tabs_create.js


Bonus points for also implementing it on Android:

http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/mobile/android/components/extensions/ext-tabs.js#308
Mentor: kmaglione+bmo
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: [design-decision-approved][tabs]
(Assignee)

Comment 2

9 months ago
Sorry for stealing your very well-documented good-first-bug, Kris, but Andy felt this would be a nice thing to land for 57.
Assignee: nobody → bob.silverberg
Comment hidden (mozreview-request)
(Assignee)

Comment 6

9 months ago
Note that while I wrote the code to implement this on Android, I wasn't sure how to write a test for it on Android. So we can either:

1. Land it as is and file a follow-up bug for an Android test.
2. Wait for me to figure out how to write an Android test for it before landing.
3. Land it without the changes for Android and file a follow-up bug to implement it on Android, with a test.
(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> Note that while I wrote the code to implement this on Android, I wasn't sure
> how to write a test for it on Android. So we can either:
> 
> 1. Land it as is and file a follow-up bug for an Android test.
> 2. Wait for me to figure out how to write an Android test for it before
> landing.
> 3. Land it without the changes for Android and file a follow-up bug to
> implement it on Android, with a test.

Lets do a followup for android, maybe rpl can help.  If he can quickly, maybe land them together.
Flags: needinfo?(lgreco)

Comment 8

9 months ago
mozreview-review
Comment on attachment 8905915 [details]
Bug 1397383 - Add loadReplace option to tabs.update,

https://reviewboard.mozilla.org/r/177714/#review182834

If we can get an android test quickly enough, cool, otherwise a followup bug for android would be preferable (unless there is a good manual STR for QA)
Attachment #8905915 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 9

9 months ago
mozreview-review-reply
Comment on attachment 8905915 [details]
Bug 1397383 - Add loadReplace option to tabs.update,

https://reviewboard.mozilla.org/r/177714/#review182834

Thanks Shane. I think I'll pull the Android pieces out of this commit based on that. If I can get some help with the Android test over the next few days I'll add a second commit. Otherwise I'll open a follow-up.

Comment 10

9 months ago
As briefly discussed with Bob over IRC, the change to the android tabs API looks right, but some of the pieces that have been used in the test are not available on Fennec (e.g. the WE history API, SessionStore.jsm and TabFlusher.jsm).

I'm absolutely happy to help find the needed replacement for the Fennec test case, but I agree with Bob that it would be more than reasonable to move out the Fennec pieces from this patch in the meantime and move them in a follow up.
Flags: needinfo?(lgreco)
Comment hidden (mozreview-request)

Comment 12

9 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/442345c31f98
Add loadReplace option to tabs.update, r=mixedpuppy
(Assignee)

Updated

9 months ago
Blocks: 1398350
https://hg.mozilla.org/mozilla-central/rev/442345c31f98
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 14

9 months ago
Hi,

thanks for implementing this option!

The new loadReplace option for browser.tabs.update() works for disabling the back button, but I still have to call browser.history.deleteUrl() afterwards because even with loadReplace: true there is an entry in the history.
(Assignee)

Comment 15

9 months ago
I think that is expected. All we are replacing is the entry in the tab (session) history, we're not affecting the actual browser history. I'm not sure if that's something we want to do automatically in this case, as you have the option of doing it yourself if you deem it necessary (as you've indicated).

Do you have an opinion on this, Kris?
Flags: needinfo?(kmaglione+bmo)
I don't think update() with a replace arg should affect global history.

That said, I think adding moz-extension urls to global history is slightly weird.  And I also think it might be nice if moz-extension urls could opt out of being added to global history in the first place...

Comment 17

9 months ago
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #16)
> That said, I think adding moz-extension urls to global history is slightly
> weird.  And I also think it might be nice if moz-extension urls could opt
> out of being added to global history in the first place...

In bug 1314123 we decided not to do that because we've got no real idea why the moz-extension ended up in the history. It could be for a good reason. For the case of a new tab, we think it makes sense because we know why a user opened the page, that's bug 1392063.
> we've got no real idea why the moz-extension ended up in the history

Right, hence the "might be nice" bit.  But bug 1392063 covers this specific instance, for sure.  Thank you!
Keywords: dev-doc-needed
I've added a thing about loadReplace: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update

Please let me know if that covers it.
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 20

7 months ago
(In reply to Will Bamberg [:wbamberg] from comment #19)
> I've added a thing about loadReplace:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
> 
> Please let me know if that covers it.

Perfect! Thanks Will.
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.