|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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
Priority: -- → P5
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
Per https://github.com/cadeyrn/newtaboverride/issues/46#issuecomment-328011090 location.replace() doesn't actually work here, btw...
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.
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+
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.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/442345c31f98 Add loadReplace option to tabs.update, r=mixedpuppy
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
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?
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...
(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!
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.
(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.
You need to log in before you can comment on or make changes to this bug.