Closed
Bug 1397383
Opened 7 years ago
Closed 7 years ago
tabs.update should be able to do a replace load
Categories
(WebExtensions :: General, enhancement, P5)
WebExtensions
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla57
People
(Reporter: bzbarsky, Assigned: bsilverberg, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [design-decision-approved][tabs])
Attachments
(1 file)
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.
Comment 1•7 years ago
|
||
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•7 years 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
Reporter | ||
Comment 3•7 years ago
|
||
Per https://github.com/cadeyrn/newtaboverride/issues/46#issuecomment-328011090 location.replace() doesn't actually work here, btw...
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52201a23d1df3a85e5527bd5887f8c127deac1ec
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years 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.
Comment 7•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/442345c31f98 Add loadReplace option to tabs.update, r=mixedpuppy
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/442345c31f98
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 14•7 years 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•7 years 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)
Reporter | ||
Comment 16•7 years ago
|
||
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•7 years 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.
Reporter | ||
Comment 18•7 years ago
|
||
> 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!
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 19•7 years ago
|
||
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 years 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)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•6 years ago
|
status-firefox57:
fixed → ---
Flags: needinfo?(kmaglione+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•