Bug 36187 – Rename window.media to window.styleMedia

• Component: DOM
• Status: CLOSED FIXED

[Simon Fraser (smfr)]

Anne changed the CSS OM View spec to use 'styleMedia':
http://dev.w3.org/csswg/cssom-view/

[Kenneth Rohde Christiansen]

Created attachment 53872 [details]
Patch

[Kenneth Rohde Christiansen]

Simon, we need this for the release as it will be used a lot by one of our target groups, and we want them to start using the right API and not the old one.

[Kenneth Rohde Christiansen]

It seems that the class also changed name. I will do an additional patch to make that change.

[Kenneth Rohde Christiansen]

Comment on attachment 53872 [details]
Patch

I apparently changed the expected result by changing the PASS: line (after testing), thus I will have to update the expected result for one Qt test.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57924 might have broken Leopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/57924
http://trac.webkit.org/changeset/57925

[James Robinson]

I have new chromium baselines ready to go whenever this relands, FYI.

[Adam Barth]

I wonder if we need a better way to do these "enumerate the global scope" tests.  It seems like a poor design that every port needs different results based on which features they happened to have enabled.

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> I have new chromium baselines ready to go whenever this relands, FYI.

Can you attach these to the bug report; I will reland the patch (with updated results) thursday when I'm back at work.

[Kenneth Rohde Christiansen]

Created attachment 53965 [details]
Patch with updated expected results

[Kenneth Rohde Christiansen]

Comment on attachment 53965 [details]
Patch with updated expected results

Landing former patch (+ updated expected results) via commit-queue.

Updated expected results are marked as unreviewed in the ChangeLog as these do not need review.

[Eric Seidel (no email)]

Comment on attachment 53872 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 53872 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> (From update of attachment 53872 [details])
> Cleared Simon Fraser's review+ from obsolete attachment 53872 [details] so that this bug
> does not appear in http://webkit.org/pending-commit.

I wanted the last patch to this bug to be committed using the commit-queue, but it doesn't show up in pending-commit. I wanted to commit it this way to make sure that I do not break the build, but if that is not posisble I will have to either request re-review or commit it myself :-(

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 53872 [details] [details])
> > Cleared Simon Fraser's review+ from obsolete attachment 53872 [details] [details] so that this bug
> > does not appear in http://webkit.org/pending-commit.
> 
> I wanted the last patch to this bug to be committed using the commit-queue, but
> it doesn't show up in pending-commit. I wanted to commit it this way to make
> sure that I do not break the build, but if that is not posisble I will have to
> either request re-review or commit it myself :-(

Ah pending-commit is r+'ed patches. Any way to see which patches are on the commit-queue?

[Eric Seidel (no email)]

No easy way to list the patches, but you can list the bugs:
https://bugs.webkit.org/buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B&order=Last+Changed

[Kenneth Rohde Christiansen]

(In reply to comment #14)
> No easy way to list the patches, but you can list the bugs:
> https://bugs.webkit.org/buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B&order=Last+Changed

Thanks, I will bookmark that. 

Anyway, it would be nice to be able to see what number a patch is in the queue of the commit-queue, maybe as a future enhancement.

[WebKit Commit Bot]

Comment on attachment 53965 [details]
Patch with updated expected results

Clearing flags on attachment: 53965

Committed r58085: <http://trac.webkit.org/changeset/58085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Created attachment 54083 [details]
Rename Media interface to StyleMedia

Second part of the change.

[Kenneth Rohde Christiansen]

The spec also requires a rename of the interface itself.

[Early Warning System Bot]

Attachment 54083 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1802055

[WebKit Review Bot]

Attachment 54083 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/Media.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/css/Media.h:27:  #ifndef header guard has wrong style, please use: Media_h  [build/header_guard] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> Attachment 54083 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebCore/css/Media.cpp:28:  Found other header before a header this file
> implements. Should be: config.h, primary header, blank line, and then
> alphabetically sorted.  [build/include_order] [4]
> WebCore/css/Media.h:27:  #ifndef header guard has wrong style, please use:
> Media_h  [build/header_guard] [5]
> Total errors found: 2 in 18 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Funny, it complains about style issues in Media.cpp etc which actually doesn't exists anymore as the patch renames it to StyleMedia.cpp

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> Attachment 54083 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/1802055

Compiles on Qt here; it might need a clean build I'm afraid. Ossy, could you test this? I hope I won't run into such problems on all platforms.

[Kenneth Rohde Christiansen]

(In reply to comment #23)
> (In reply to comment #20)
> > Attachment 54083 [details] [details] did not build on qt:
> > Build output: http://webkit-commit-queue.appspot.com/results/1802055
> 
> Compiles on Qt here; it might need a clean build I'm afraid. Ossy, could you
> test this? I hope I won't run into such problems on all platforms.

Strange it cannot find StyleMedia.idl, it is like the patch doesn't apply correctly. The patch says:

diff --git a/WebCore/css/Media.idl b/WebCore/css/StyleMedia.idl
similarity index 93%
rename from WebCore/css/Media.idl
rename to WebCore/css/StyleMedia.idl
index 1bf5900..7be35cc 100644
--- a/WebCore/css/Media.idl
+++ b/WebCore/css/StyleMedia.idl

[Kenneth Rohde Christiansen]

Created attachment 54087 [details]
Patch

[WebKit Review Bot]

Attachment 54087 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/Media.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/css/Media.h:27:  #ifndef header guard has wrong style, please use: Media_h  [build/header_guard] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 54087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1807043

[Kenneth Rohde Christiansen]

webkit-patch apply-attachment 54087 [details]
M	LayoutTests/ChangeLog
M	LayoutTests/fast/dom/prototype-inheritance-2-expected.txt
M	LayoutTests/platform/win/fast/dom/prototype-inheritance-2-expected.txt
M	WebCore/Android.derived.jscbindings.mk
M	WebCore/Android.derived.v8bindings.mk
M	WebCore/ChangeLog
M	WebCore/GNUmakefile.am
M	WebCore/WebCore.gypi
M	WebCore/WebCore.pri
M	WebCore/WebCore.pro
M	WebCore/WebCore.vcproj/WebCore.vcproj
M	WebCore/WebCore.xcodeproj/project.pbxproj
D	WebCore/css/Media.cpp
D	WebCore/css/Media.h
D	WebCore/css/Media.idl
M	WebCore/page/DOMWindow.cpp
M	WebCore/page/DOMWindow.h
M	WebCore/page/DOMWindow.idl

It deletes the files to be renamed and it doesnt create any new files.

[Kenneth Rohde Christiansen]

Can anyone help me creating a valid diff which includes renaming from a git repo?

If not, I cannot make use of the bots and will have to land this manually at some point. If I do that I will do so in the morning so that any breakage should be fixed before California working hours.

[WebKit Review Bot]

Attachment 54087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1880006

[WebKit Review Bot]

Attachment 54087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1834043

[Laszlo Gombos]

Comment on attachment 54087 [details]
Patch

Patch looks good, but the diff for renaming the files is not understood by our tools. This patch can only be landed manually or has to be regenerated from an svn repo.

[Laszlo Gombos]

Comment on attachment 54087 [details]
Patch

As Kenneth promised to land it manually correctly.

[David Kilzer (:ddkilzer)]

(In reply to comment #32)
> (From update of attachment 54087 [details])
> Patch looks good, but the diff for renaming the files is not understood by our
> tools. This patch can only be landed manually or has to be regenerated from an
> svn repo.

(In reply to comment #33)
> (From update of attachment 54087 [details])
> As Kenneth promised to land it manually correctly.

Why was this landed (in r58168) when nearly every try bot turned up red on the final patch?!

[David Kilzer (:ddkilzer)]

Patch landed in r58168.  <http://trac.webkit.org/changeset/58168>

Follow-up build and test results fixes (with more likely on the way):
<http://trac.webkit.org/changeset/58169>
<http://trac.webkit.org/changeset/58170>
<http://trac.webkit.org/changeset/58171>
<http://trac.webkit.org/changeset/58172>
<http://trac.webkit.org/changeset/58173>
<http://trac.webkit.org/changeset/58174>

[David Kilzer (:ddkilzer)]

(In reply to comment #35)
> Patch landed in r58168.  <http://trac.webkit.org/changeset/58168>
> 
> Follow-up build and test results fixes (with more likely on the way):
> <http://trac.webkit.org/changeset/58169>
> <http://trac.webkit.org/changeset/58170>
> <http://trac.webkit.org/changeset/58171>
> <http://trac.webkit.org/changeset/58172>
> <http://trac.webkit.org/changeset/58173>
> <http://trac.webkit.org/changeset/58174>

<http://trac.webkit.org/changeset/58175>

[Kenneth Rohde Christiansen]

Comment on attachment 54087 [details]
Patch

Landed in 58168 with lots of trouble.

Thanks for everyone helping me get the bots green as fast as possible.

[Kenneth Rohde Christiansen]

> Why was this landed (in r58168) when nearly every try bot turned up red on the
> final patch?!

My mistake. As it was a rename it wouldn't apply the patch to run on the EWS bots (see comment 28), thus I decided to land it in the morning and fix the issues as fast as possible, though I hadn't imagined that there would be so many.

Sorry for any trouble, and thank you very much for the help.

[Kenneth Rohde Christiansen]

(In reply to comment #38)
> Sorry for any trouble, and thank you very much for the help.

I will move to Mac OS X for development so that I can test better patches as this one in the future.

[David Kilzer (:ddkilzer)]

(In reply to comment #29)
> Can anyone help me creating a valid diff which includes renaming from a git
> repo?
> 
> If not, I cannot make use of the bots and will have to land this manually at
> some point. If I do that I will do so in the morning so that any breakage
> should be fixed before California working hours.

The svn-create-patch script does this.  However, git patches with renames don't work yet.  See Bug 32834.

[Kenneth Rohde Christiansen]

Please cherry-pick this one Simon.

[Kenneth Rohde Christiansen]

(In reply to comment #41)
> Please cherry-pick this one Simon.

I mean Simon Hausmann. Sorry about the confusion Simon Fraser :-)

[Simon Hausmann]

Revision r58085 cherry-picked into qtwebkit-2.0 with commit 5b263f2040886fc71100cb507d2e80f963021496
Revision r58168 cherry-picked into qtwebkit-2.0 with commit 35a9c2373f8260aab20d3f1b1fcb862fb0f35694