Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalise RootedVec::<JS<T>>::r as [JS<T>]::r #11171

Closed
wants to merge 3 commits into from
Closed

Conversation

@nox
Copy link
Member

nox commented May 13, 2016

This change is Reviewable

@highfive
Copy link

highfive commented May 13, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/global.rs, components/script/dom/htmldatalistelement.rs, components/script/dom/htmlformcontrolscollection.rs, components/script/dom/htmlimageelement.rs, components/script/dom/navigator.rs, components/script/dom/htmlcollection.rs, components/script/dom/treewalker.rs, components/script/dom/nodeiterator.rs, components/script/parse/mod.rs, components/script/dom/filereader.rs, components/script/dom/htmlselectelement.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/servohtmlparser.rs, components/script/dom/blob.rs, components/script/dom/bluetoothdevice.rs, components/script/devtools.rs, components/script/dom/bluetoothremotegattserver.rs, components/script/dom/document.rs, components/script/dom/domquad.rs, components/script/dom/bindings/refcounted.rs, components/script/dom/eventtarget.rs, components/script/timers.rs, components/script/dom/bindings/callback.rs, components/script/dom/domstringmap.rs, components/script/dom/htmltablecellelement.rs, components/script/dom/webglprogram.rs, components/script/dom/htmlobjectelement.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/htmlbuttonelement.rs, components/script/dom/activation.rs, components/script/dom/htmloutputelement.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/bluetoothremotegattservice.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/htmldetailselement.rs, components/script/dom/comment.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bindings/js.rs, components/script/dom/bluetoothremotegattcharacteristic.rs, components/script/dom/domimplementation.rs, components/script/dom/testbinding.rs, components/script/dom/performancetiming.rs, components/script/dom/htmliframeelement.rs, components/script/dom/console.rs, components/script/dom/url.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/worker.rs, components/script/dom/htmlformelement.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/documentfragment.rs, components/script/dom/websocket.rs, components/script/dom/domparser.rs, components/script/dom/domtokenlist.rs, components/script/dom/formdata.rs
  • @emilio: components/script/dom/webglprogram.rs, components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented May 13, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member Author

nox commented May 13, 2016

This supersedes #11162.

@nox
Copy link
Member Author

nox commented May 13, 2016

r? @Ms2ger

@highfive highfive assigned Ms2ger and unassigned KiChjang May 13, 2016
@nox nox force-pushed the nox:rootedreference branch from d419fcb to 8b6435e May 13, 2016
@highfive
Copy link

highfive commented May 13, 2016

New code was committed to pull request.

@nox nox changed the title Generalise RootedVec::<JS<T>::r as [JS<T>]::r Generalise RootedVec::<JS<T>>::r as [JS<T>]::r May 13, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented May 13, 2016

Fine by me, r? @jdm for the extra uses.

@highfive highfive assigned jdm and unassigned Ms2ger May 13, 2016
@KiChjang
Copy link
Member

KiChjang commented May 13, 2016

Fails tidy:

Checking files for tidiness...

./components/script/dom/bindings/callback.rs:9: use statement is not in alphabetical order

    expected: dom::bindings::js::RootedReference

    found: dom::bindings::global::global_root_from_object
@nox nox force-pushed the nox:rootedreference branch from 8b6435e to db59539 May 13, 2016
@highfive
Copy link

highfive commented May 13, 2016

New code was committed to pull request.

@cbrewster cbrewster removed the S-fails-tidy label May 13, 2016
@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2016

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

nox added 3 commits May 13, 2016
It now becomes RootedReference<'root> and includes an associated type for
the return type of its 'r' method.

This removes the need for OptionalRootedReference.
@nox nox removed the S-needs-rebase label May 16, 2016
@nox nox force-pushed the nox:rootedreference branch from db59539 to a66957e May 16, 2016
@highfive
Copy link

highfive commented May 16, 2016

New code was committed to pull request.

@jdm
Copy link
Member

jdm commented May 16, 2016

I'm not convinced that this is a strict improvement. Conceptually the implementation is nicer to read, but the extra import required for every file that needs to interact with a GlobalRoot is a papercut that outweighs that.

@jdm jdm removed the S-awaiting-review label May 16, 2016
@bors-servo
Copy link
Contributor

bors-servo commented May 17, 2016

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

@notriddle
Copy link
Contributor

notriddle commented Sep 12, 2016

So, what's the status on this one? Is it just a change we don't want to make at all? Or should it be rebased and fixed up in some way? @jdm?

@jdm
Copy link
Member

jdm commented Sep 12, 2016

Previous discussions have not convinced me that this change is worth merging.

@jdm jdm closed this Sep 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.