Browsing context names #20036

paavininanda · 2018-02-13T09:24:40Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14453

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-02-13T09:24:46Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

highfive · 2018-02-13T09:24:47Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmliframeelement.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/windowproxy.rs, components/script/dom/webidls/HTMLIFrameElement.webidl, components/script/dom/window.rs and 1 more
@fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/windowproxy.rs, components/script/dom/webidls/HTMLIFrameElement.webidl, components/script/dom/window.rs and 1 more
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/windowproxy.rs, components/script/dom/webidls/HTMLIFrameElement.webidl, components/script/dom/window.rs and 1 more

highfive · 2018-02-13T09:24:48Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

paavininanda · 2018-02-13T09:31:38Z

r? @jdm

bors-servo · 2018-02-13T14:55:00Z

☔ The latest upstream changes (presumably #19975) made this pull request unmergeable. Please resolve the merge conflicts.

cbrewster

Thanks for working on this!
There are some changes that need to made to make sure browsing context names are updated in all the appropriate places (windows/iframes in other script threads)

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/webidls/HTMLIFrameElement.webidl

@@ -10,9 +10,8 @@ interface HTMLIFrameElement : HTMLElement {
  // [CEReactions]
  //         attribute DOMString srcdoc;

-  // https://github.com/servo/servo/issues/14453
  // [CEReactions]


Uncomment the [CEReactions] annotation here.

This is still commented out.

I read it wrong :/ long back.

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/window.rs

@@ -1019,6 +1019,20 @@ impl WindowMethods for Window {
    fn TestRunner(&self) -> DomRoot<TestRunner> {
        self.test_runner.or_init(|| TestRunner::new(self.upcast()))
    }
+
+    // https://fetch.spec.whatwg.org/multipage/browsers.html#browsing-context-names


This should be https://html.spec.whatwg.org/multipage/#dom-name

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/window.rs

+    }
+
+    // https://fetch.spec.whatwg.org/multipage/browsers.html#browsing-context-names
+    fn Name(&self) -> DOMString {


This should be https://html.spec.whatwg.org/multipage/#dom-name

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/htmliframeelement.rs

@@ -457,6 +459,18 @@ impl HTMLIFrameElementMethods for HTMLIFrameElement {
    make_getter!(FrameBorder, "frameborder");
    // https://html.spec.whatwg.org/multipage/#other-elements,-attributes-and-apis:attr-iframe-frameborder
    make_setter!(SetFrameBorder, "frameborder");
+
+    fn SetName(&self, name: DOMString) {


Add spec comment: https://html.spec.whatwg.org/multipage/#dom-iframe-name

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/htmliframeelement.rs

+        }
+    }
+
+    fn Name(&self) -> DOMString {


Add spec comment: https://html.spec.whatwg.org/multipage/#dom-iframe-name

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/htmliframeelement.rs

@@ -457,6 +459,18 @@ impl HTMLIFrameElementMethods for HTMLIFrameElement {
    make_getter!(FrameBorder, "frameborder");
    // https://html.spec.whatwg.org/multipage/#other-elements,-attributes-and-apis:attr-iframe-frameborder
    make_setter!(SetFrameBorder, "frameborder");
+
+    fn SetName(&self, name: DOMString) {
+        window_from_node(self).window_proxy().set_name(name.clone());


We probably don't want to do this as it would set the name of the browsing context of the window that contains the iframe. We just want to set the name of the content window's browsing context. (The browsing context inside the iframe).

Note: If the child window is on a different script thread, we must send a message to the constellation which will then send a message back to the script thread with child window and update the name there.

So do we need to send a message to constellation using script_traits::ScriptMsg ? If yes then which method of all the available methods is to be used ?

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/window.rs

+    // https://fetch.spec.whatwg.org/multipage/browsers.html#browsing-context-names
+    fn SetName(&self, name: DOMString) {
+        self.window_proxy().set_name(name.clone());
+        if let Some(parent) = self.window_proxy().parent() {


I don't believe we should be setting the parent browsing context's name, just the browsing context of this window.

cbrewster · 2018-02-13T19:57:02Z

components/script/dom/windowproxy.rs

@@ -55,6 +57,8 @@ pub struct WindowProxy {
    /// In the case that this is a top-level window, this is our id.
    top_level_browsing_context_id: TopLevelBrowsingContextId,

+    /// The name of the browsing context
+    name: DomRefCell<DOMString>,


I'm not sure if this is something that should be stored on the WindowProxy or if it will need to be stored in the constellation on the BrowsingContext. cc @asajeffrey @jdm

The comment in the issue said so 😅 #14453

It's quite possible it should be stored here, just wanted to double check 😄

I suspect we can get away with only storing the name in script/, rather than duplicating it in the constellation.

@jdm: not sure about that, what about when an existing bc has a document opened in a new (to it) script thread?

asajeffrey · 2018-02-14T18:07:05Z

Hi there, this is all looking good for the case of immutable access to the browsing context name.

The problem as mentioned by @cbrewster is the mutable case, since this would involve sending a message to the constellation to notify it of the name change. Moreover, the name change is expected to be synchronous, which is probably going to make the logic quite torturous.

Perhaps the thing to do is just implement the immutable case, mark the attribute read-only in the webidl, with a comment referencing an issue to implement setting the BC name?

Seem like a good idea? @paavininanda @jdm?

jdm · 2018-02-14T18:59:16Z

https://mozilla.logbot.info/servo/20180214#c14302329-c14302486 is a discussion I had with asajeffrey. I think they're right that we should put off the message-sending part of this; it will require a bit more thought to make sure we're not missing anything. We don't need to make the name read-only, though; for the case we care about the most, the implementation that modifies the name stored in the window proxy should work for our same-origin iframe case.

jdm

One other change that we'll need (to support changing the name attribute on an iframe):

in attribute_mutated in htmliframeelement.rs, add a branch for the "name" attribute and do:

let value = mutation.new_value(attr).map_or("", |v| &v);
self.SetName(DOMString::from(value.to_owned()));

jdm · 2018-02-14T19:14:48Z

components/script/dom/htmliframeelement.rs

@@ -67,6 +67,7 @@ enum ProcessingMode {
 pub struct HTMLIFrameElement {
    htmlelement: HTMLElement,
    top_level_browsing_context_id: Cell<Option<TopLevelBrowsingContextId>>,
+    name: DomRefCell<DOMString>,


We should not need to store a name in this data structure.

jdm · 2018-02-14T19:14:48Z

components/script/dom/htmliframeelement.rs

+    }
+
+    fn Name(&self) -> DOMString {
+        window_from_node(self).window_proxy().get_name()


Use self.GetContentWindow(), and if it exists we can call get_name() on it.

jdm · 2018-02-14T19:14:48Z

components/script/dom/windowproxy.rs

@@ -84,6 +88,7 @@ impl WindowProxy {
            reflector: Reflector::new(),
            browsing_context_id: browsing_context_id,
            top_level_browsing_context_id: top_level_browsing_context_id,
+            name: DomRefCell::new(DOMString::new()),


To support the case of <iframe src="whatever.html" name="hello">, we need to do this:

let name = frame_element.map_or(DOMString::new(), |e| e.get_string_attribute(&local_name!("name"))); ... name: DomRefCell::new(name),

paavininanda · 2018-02-15T17:05:06Z

let value = mutation.new_value(attr).map_or("", |v| &v);

This is giving an error -
borrowed value does not live long enough
v dropped here while still borrowed

jdm · 2018-02-15T17:07:28Z

Try

let new_value = mutation.new_value(attr);
let value = new_value.as_ref().map_or("", |v| &v);

jdm

Almost there! Once the [CEReactions] attribute is added, we can run this through the tryserver and hopefully get a long list of new test results!

jdm · 2018-02-22T19:05:34Z

components/script/dom/webidls/HTMLIFrameElement.webidl

-  // https://github.com/servo/servo/issues/14453
-  // [CEReactions]
-  // attribute DOMString name;
+  attribute DOMString name;


We need to keep the [CEReactions] attribute.

jdm · 2018-02-22T19:05:34Z

components/script/dom/htmliframeelement.rs

+    // https://html.spec.whatwg.org/multipage/#dom-iframe-name
+    fn SetName(&self, name: DOMString) {
+        if let Some(window) = self.GetContentWindow() {
+            let mut child_window = &*window;


Is this line actually necessary? If it is, we should at least be able to remove the mut.

jdm · 2018-02-22T19:05:34Z

components/script/dom/htmliframeelement.rs

+    // https://html.spec.whatwg.org/multipage/#dom-iframe-name
+    fn Name(&self) -> DOMString {
+        if let Some(window) = self.GetContentWindow() {
+            let mut child_window = &*window;


Same comment as previous.

paavininanda · 2018-02-23T04:47:19Z

Didn't work, still getting the same error.

jdm · 2018-02-23T15:55:28Z

Oh! I ran it locally and realized that the test is written poorly (since it uses JS strict mode but does not declare var name before assigning it), but it happens to work in browsers because of the name attribute on the Window object. That means that the test was not working at all before these changes, and now is showing us the actual test results that we care about. That means you can go ahead and save the logs of running the tests and update the expected test results:
wget http://build.servo.org/builders/linux-rel-wpt/builds/8021/steps/test__1/logs/test-wpt.log/text -o log1 && ./mach update-wpt log1 and wget http://build.servo.org/builders/linux-rel-css/builds/8007/steps/test/logs/test-wpt.log/ -o log2 && ./mach update-wpt log2.

jdm · 2018-02-23T15:56:21Z

Oh, and you can undo the change to gl-uniformmatrix4fv.html before committing.

paavininanda · 2018-02-23T18:03:23Z

tests/wpt/mozilla/meta/MANIFEST.json

+    [
+     {}
+    ]
+   ],


I think I'll have to manually remove all the ./.DS_Store part

You should just be able to revert all of the MANIFEST.json changes entirely.

jdm · 2018-02-25T20:25:11Z

The changes to gl-uniformmatrix4fv.html.ini are missing; otherwise this looks good.

jdm · 2018-02-25T20:26:39Z

You should be able to run ./mach test-wpt tests/wpt/mozilla/tests/webgl/conformance-1.0.3/conformance/uniforms/gl-uniformmatrix4fv.html --no-pause-after-test --log-raw /tmp/wpt then ./mach update-wpt /tmp/wpt to update the ini file correctly.


        Adding Name and SetName functions for window

jdm · 2018-02-26T02:26:04Z

@bors-servo r+

bors-servo · 2018-02-26T02:26:05Z

📌 Commit b9f7aa4 has been approved by jdm

bors-servo · 2018-02-26T02:26:10Z

⌛ Testing commit b9f7aa4 with merge 7de2043...


        Auto merge of #20036 - paavininanda:BrowsingNames, r=jdm

Browsing context names  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14453  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20036)

bors-servo · 2018-02-26T02:44:45Z

💔 Test failed - mac-rel-wpt1

jdm · 2018-02-26T04:10:50Z

@bors-servo retry

#20087

bors-servo · 2018-02-26T04:10:53Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt1...

bors-servo · 2018-02-26T05:00:18Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 7de2043 to master...

highfive assigned metajack Feb 13, 2018

highfive added the S-awaiting-review label Feb 13, 2018

highfive assigned jdm and unassigned metajack Feb 13, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch from 53649e8 to 577b78d Feb 13, 2018

paavininanda changed the title ~~Browsing names~~ Browsing context names Feb 13, 2018

highfive added the S-needs-rebase label Feb 13, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch 2 times, most recently from 276d7cd to b2fc4e0 Feb 13, 2018

cbrewster requested changes Feb 13, 2018

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review labels Feb 13, 2018

jdm requested changes Feb 14, 2018

View changes

paavininanda force-pushed the paavininanda:BrowsingNames branch from b2fc4e0 to 73467e9 Feb 15, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 15, 2018

jdm requested changes Feb 22, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Feb 22, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch from 73467e9 to bf75e6b Feb 22, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch from e45d284 to e6e94e3 Feb 23, 2018

highfive removed the S-tests-failed label Feb 23, 2018

paavininanda reviewed Feb 23, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 23, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch from e6e94e3 to d531683 Feb 23, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 23, 2018

paavininanda force-pushed the paavininanda:BrowsingNames branch 2 times, most recently from 3608bd7 to 88c5fdd Feb 25, 2018

Adding Name and SetName functions for window

Loading status checks…

b9f7aa4

paavininanda force-pushed the paavininanda:BrowsingNames branch from 68d750f to b9f7aa4 Feb 26, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 26, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 26, 2018

bors-servo merged commit b9f7aa4 into servo:master Feb 26, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Join GitHub today

Browsing context names #20036

Browsing context names #20036

Conversation

paavininanda commented Feb 13, 2018 • edited by SimonSapin

highfive commented Feb 13, 2018

highfive commented Feb 13, 2018

highfive commented Feb 13, 2018

paavininanda commented Feb 13, 2018

bors-servo commented Feb 13, 2018

cbrewster left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cbrewster Feb 13, 2018 • edited

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Feb 14, 2018

jdm commented Feb 14, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paavininanda commented Feb 15, 2018

jdm commented Feb 15, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paavininanda commented Feb 23, 2018

jdm commented Feb 23, 2018

jdm commented Feb 23, 2018

This comment has been minimized.

paavininanda Feb 23, 2018 • edited

This comment has been minimized.

jdm commented Feb 25, 2018

jdm commented Feb 25, 2018

jdm commented Feb 26, 2018

bors-servo commented Feb 26, 2018

bors-servo commented Feb 26, 2018

bors-servo commented Feb 26, 2018

jdm commented Feb 26, 2018

bors-servo commented Feb 26, 2018

bors-servo commented Feb 26, 2018

paavininanda commented Feb 13, 2018 •

edited by SimonSapin

cbrewster left a comment •

edited

cbrewster Feb 13, 2018 •

edited

paavininanda Feb 23, 2018 •

edited