Skip to content

Commit

Permalink
Auto merge of #10100 - paulrouget:historyOnLocationChange, r=paulrouget
Browse files Browse the repository at this point in the history
Add history information to mozbrowserlocationchange event

This is a change in the Browser API itself.

Before, on `mozbrowserlocationchange`, we would call `getCanGoBack()` and `getCanGoForward()`. Two asynchronous methods called on an event, which doesn't make much sense, especially because we already know on `mozbrowserlocationchange` if we can go back/forward. So here I'm adding 2 new properties to the event to tell if the iframe can go back/forward.

The way `event.detail` is defined also changed. Before, `event.detail` was a string (the new uri), now it's an object (`{uri:String,canGoBack:bool,canGoForward:bool}`).

This is one of the design flaw of the early Browser API: not using objects for the detail property, making it hard to extend the event payload.

So that makes this event not backward compatible. We can:
1. just don't care. It's up to the client to test if event.detail is a string or not if it needs to be compatible with Gecko
2. fix it in Gecko. The client will still have to test `event.detail` to make it compatible with older version of gecko
3. rename `mozbrowserlocationchange` to something else (`mozbrowserlocationchange2` ?)

Please advise.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10100)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Mar 22, 2016
2 parents 9a8ba23 + 6577409 commit db63aa4
Show file tree
Hide file tree
Showing 8 changed files with 90 additions and 8 deletions.
7 changes: 5 additions & 2 deletions components/compositing/constellation.rs
Expand Up @@ -1655,8 +1655,11 @@ impl<LTF: LayoutThreadFactory, STF: ScriptThreadFactory> Constellation<LTF, STF>
// If this is an iframe, then send the event with new url
if let Some((containing_pipeline_id, subpage_id, url)) = event_info {
let parent_pipeline = self.pipeline(containing_pipeline_id);
parent_pipeline.trigger_mozbrowser_event(subpage_id,
MozBrowserEvent::LocationChange(url));
let frame_id = *self.pipeline_to_frame_map.get(&pipeline_id).unwrap();
let can_go_backward = !self.frame(frame_id).prev.is_empty();
let can_go_forward = !self.frame(frame_id).next.is_empty();
let event = MozBrowserEvent::LocationChange(url, can_go_backward, can_go_forward);
parent_pipeline.trigger_mozbrowser_event(subpage_id, event);
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion components/script/dom/htmliframeelement.rs
Expand Up @@ -6,6 +6,7 @@ use document_loader::{LoadType, LoadBlocker};
use dom::attr::{Attr, AttrValue};
use dom::bindings::cell::DOMRefCell;
use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserElementIconChangeEventDetail;
use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserElementLocationChangeEventDetail;
use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserElementSecurityChangeDetail;
use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserShowModalPromptEventDetail;
use dom::bindings::codegen::Bindings::HTMLIFrameElementBinding;
Expand Down Expand Up @@ -323,9 +324,16 @@ impl MozBrowserEventDetailBuilder for HTMLIFrameElement {
mixedState: None,
}.to_jsval(cx, rval);
}
MozBrowserEvent::LocationChange(ref string) | MozBrowserEvent::TitleChange(ref string) => {
MozBrowserEvent::TitleChange(ref string) => {
string.to_jsval(cx, rval);
}
MozBrowserEvent::LocationChange(uri, can_go_back, can_go_forward) => {
BrowserElementLocationChangeEventDetail {
uri: Some(DOMString::from(uri)),
canGoBack: Some(can_go_back),
canGoForward: Some(can_go_forward),
}.to_jsval(cx, rval);
}
MozBrowserEvent::IconChange(rel, href, sizes) => {
BrowserElementIconChangeEventDetail {
rel: Some(DOMString::from(rel)),
Expand Down
6 changes: 6 additions & 0 deletions components/script/dom/webidls/BrowserElement.webidl
Expand Up @@ -54,6 +54,12 @@ dictionary BrowserElementSecurityChangeDetail {
boolean mixedContent;
};

dictionary BrowserElementLocationChangeEventDetail {
DOMString uri;
boolean canGoBack;
boolean canGoForward;
};

dictionary BrowserElementIconChangeEventDetail {
DOMString rel;
DOMString href;
Expand Down
4 changes: 2 additions & 2 deletions components/script_traits/lib.rs
Expand Up @@ -424,7 +424,7 @@ pub enum MozBrowserEvent {
/// Sent when the browser `<iframe>` starts to load a new page.
LoadStart,
/// Sent when a browser `<iframe>`'s location changes.
LocationChange(String),
LocationChange(String, bool, bool),
/// Sent when window.open() is called within a browser `<iframe>`.
OpenWindow,
/// Sent when the SSL state changes within a browser `<iframe>`.
Expand All @@ -451,7 +451,7 @@ impl MozBrowserEvent {
MozBrowserEvent::IconChange(_, _, _) => "mozbrowsericonchange",
MozBrowserEvent::LoadEnd => "mozbrowserloadend",
MozBrowserEvent::LoadStart => "mozbrowserloadstart",
MozBrowserEvent::LocationChange(_) => "mozbrowserlocationchange",
MozBrowserEvent::LocationChange(_, _, _) => "mozbrowserlocationchange",
MozBrowserEvent::OpenWindow => "mozbrowseropenwindow",
MozBrowserEvent::SecurityChange(_) => "mozbrowsersecuritychange",
MozBrowserEvent::ShowModalPrompt(_, _, _, _) => "mozbrowsershowmodalprompt",
Expand Down
6 changes: 6 additions & 0 deletions tests/wpt/mozilla/meta/MANIFEST.json
Expand Up @@ -6024,6 +6024,12 @@
"url": "/_mozilla/mozilla/mozbrowser/mozbrowsericonchange_event.html"
}
],
"mozilla/mozbrowser/mozbrowserlocationchange_event.html": [
{
"path": "mozilla/mozbrowser/mozbrowserlocationchange_event.html",
"url": "/_mozilla/mozilla/mozbrowser/mozbrowserlocationchange_event.html"
}
],
"mozilla/mozbrowser/mozbrowsersecuritychange_event.html": [
{
"path": "mozilla/mozbrowser/mozbrowsersecuritychange_event.html",
Expand Down
4 changes: 2 additions & 2 deletions tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_goback.html
Expand Up @@ -19,8 +19,8 @@
iframe.src = url1;

iframe.addEventListener("mozbrowserlocationchange", e => {
locations.push(e.detail);
if (e.detail == url2) {
locations.push(e.detail.uri);
if (e.detail.uri == url2) {
iframe.goBack();
}
if (locations.length == expected_locations.length) {
Expand Down
@@ -0,0 +1,59 @@
<head>
<title>Browser API; mozbrowserlocationchange event</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
</head>
<body>
<script>

async_test(function(t) {

var url1 = "data:,1";
var url2 = "data:,2";
var url3 = "data:,3";

var received_events = []
var expected_events = [
url1, false, false,
url2, true, false,
url3, true, false,
url2, true, true,
url1, false, true,
url2, true, true,
url3, true, false,
];

var iframe = document.createElement("iframe");
iframe.mozbrowser = "true";
iframe.src = url1;

var actions = [
function() {iframe.src = url2},
function() {iframe.src = url3},
function() {iframe.goBack()},
function() {iframe.goBack()},
function() {iframe.goForward()},
function() {iframe.goForward()},
];

var action_idx = 0;

iframe.addEventListener("mozbrowserlocationchange", e => {
received_events.push(e.detail.uri);
received_events.push(e.detail.canGoBack);
received_events.push(e.detail.canGoForward);

if (action_idx < actions.length) {
actions[action_idx++]();
} else {
assert_array_equals(received_events, expected_events);
t.done();
}
});

document.body.appendChild(iframe);

});

</script>
</body>
2 changes: 1 addition & 1 deletion tests/wpt/mozilla/tests/mozilla/mozbrowser/redirect.html
Expand Up @@ -10,7 +10,7 @@
iframe.mozbrowser = "true";
iframe.src = "redirect_init.html?pipe=status(302)|header(Location,redirect_final.html)";
iframe.addEventListener("mozbrowserlocationchange", t.step_func(e => {
assert_equals(e.detail, new URL("redirect_final.html", location).href);
assert_equals(e.detail.uri, new URL("redirect_final.html", location).href);
t.done();
}));
document.body.appendChild(iframe);
Expand Down

0 comments on commit db63aa4

Please sign in to comment.