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

Implement [Func] #11308

Merged
merged 12 commits into from May 27, 2016
Merged

Implement [Func] #11308

merged 12 commits into from May 27, 2016

Conversation

@nox
Copy link
Member

nox commented May 20, 2016

First part of #11292, this just includes support of [Func].


This change is Reviewable

@highfive
Copy link

highfive commented May 20, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/utils.rs, components/script/dom/bindings/guard.rs, components/script/dom/webidls/TestBinding.webidl, components/script/dom/bindings/interface.rs
@highfive
Copy link

highfive commented May 20, 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 20, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned SimonSapin May 20, 2016
@nox
Copy link
Member Author

nox commented May 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

Trying commit dd16933 with merge 524bf1a...

bors-servo added a commit that referenced this pull request May 20, 2016
Implement [Func]

First part of #11292, this just includes support of `[Func]`.
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

Trying commit dd16933 with merge ef4e165...

bors-servo added a commit that referenced this pull request May 20, 2016
Implement [Func]

First part of #11292, this just includes support of `[Func]`.

<!-- 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/11308)
<!-- Reviewable:end -->
@nox nox force-pushed the nox:guarded branch from dd16933 to 3e2a6e4 May 20, 2016
@highfive
Copy link

highfive commented May 20, 2016

New code was committed to pull request.

@nox
Copy link
Member Author

nox commented May 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

Trying commit 3e2a6e4 with merge b09a890...

bors-servo added a commit that referenced this pull request May 20, 2016
Implement [Func]

First part of #11292, this just includes support of `[Func]`.

<!-- 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/11308)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2016

@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@nox nox force-pushed the nox:guarded branch from bc1fe06 to 9322c37 May 23, 2016
@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@nox nox force-pushed the nox:guarded branch from 9322c37 to 0cff169 May 23, 2016
@highfive
Copy link

highfive commented May 23, 2016

New code was committed to pull request.

@nox
Copy link
Member Author

nox commented May 23, 2016

I've added a few commits that conditionally-define HTMLIFrameElement.mozbrowser.

@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

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

@nox nox force-pushed the nox:guarded branch 2 times, most recently from 79493fa to 7d5ffec May 24, 2016
@highfive
Copy link

highfive commented May 24, 2016

New code was committed to pull request.

@jdm
Copy link
Member

jdm commented May 26, 2016

Nice work! I appreciated how the commits were broken up.
-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, 5 of 5 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 5 of 5 files at r9, 2 of 2 files at r10, 6 of 6 files at r11, 3 of 3 files at r12, 17 of 17 files at r13, 1 of 1 files at r14, 2 of 2 files at r15, 3 of 3 files at r16, 5 of 5 files at r17, 1 of 1 files at r18, 1 of 1 files at r19, 1 of 1 files at r20, 5 of 5 files at r21, 2 of 2 files at r22, 6 of 6 files at r23, 3 of 3 files at r24, 1 of 1 files at r25, 17 of 17 files at r26, 1 of 1 files at r27, 2 of 2 files at r28, 3 of 3 files at r29, 5 of 5 files at r30, 1 of 1 files at r31, 1 of 1 files at r32, 1 of 1 files at r33, 5 of 5 files at r34, 2 of 2 files at r35, 6 of 6 files at r36, 3 of 3 files at r37, 4 of 4 files at r38, 3 of 3 files at r39.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/script/dom/bindings/codegen/CodegenRust.py, line 1336 [r1] (raw file):

    assert pref is None or isinstance(pref, str)
    assert func is None or isinstance(func, str)
    if pref is not None:

if pref:


components/script/dom/bindings/codegen/CodegenRust.py, line 1340 [r39] (raw file):

    if pref is not None:
        return 'Condition::Pref("%s")' % pref
    if func is not None:

if func:


Comments from Reviewable

@nox nox force-pushed the nox:guarded branch from 7d5ffec to ca83b85 May 26, 2016
@highfive
Copy link

highfive commented May 26, 2016

New code was committed to pull request.

@jdm
Copy link
Member

jdm commented May 26, 2016

As far as I can see, we're not actually testing the new properties and methods that were added to TestBinding. We should add them to the interface_member_exposed.html test.
-S-awaiting-review +S-needs-tests

Previously, highfive wrote…

New code was committed to pull request.


Reviewed 17 of 17 files at r40, 1 of 1 files at r41, 2 of 2 files at r42, 3 of 3 files at r43, 5 of 5 files at r44, 1 of 1 files at r45, 1 of 1 files at r46, 1 of 1 files at r47, 5 of 5 files at r48, 2 of 2 files at r49, 6 of 6 files at r50, 3 of 3 files at r51.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nox nox force-pushed the nox:guarded branch from ca83b85 to 694deab May 26, 2016
@highfive
Copy link

highfive commented May 26, 2016

New code was committed to pull request.

@nox
Copy link
Member Author

nox commented May 26, 2016

@jdm I added the test you wanted.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 27, 2016

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

📌 Commit 694deab has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

Testing commit 694deab with merge 073c5e3...

bors-servo added a commit that referenced this pull request May 27, 2016
Implement [Func]

First part of #11292, this just includes support of `[Func]`.

<!-- 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/11308)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

@bors-servo bors-servo merged commit 694deab into servo:master May 27, 2016
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
@bors-servo bors-servo mentioned this pull request May 27, 2016
4 of 5 tasks complete
@nox nox mentioned this pull request Mar 1, 2017
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

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