Bug 36187

Summary: Rename window.media to window.styleMedia
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: DOMAssignee: Kenneth Rohde Christiansen <kenneth>
Status: CLOSED FIXED    
Severity: Normal CC: abarth, commit-queue, ddkilzer, dglazkov, dino, eric, gustavo, hausmann, jamesr, Ms2ger, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Patch
none
Patch with updated expected results
none
Rename Media interface to StyleMedia
none
Patch laszlo.gombos: review+, kenneth: commit-queue-

Description Simon Fraser (smfr) 2010-03-16 13:43:25 PDT
Anne changed the CSS OM View spec to use 'styleMedia':
http://dev.w3.org/csswg/cssom-view/
Comment 1 Kenneth Rohde Christiansen 2010-04-20 13:21:59 PDT
Created attachment 53872 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2010-04-20 13:22:55 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2010-04-20 13:26:39 PDT
It seems that the class also changed name. I will do an additional patch to make that change.
Comment 4 Kenneth Rohde Christiansen 2010-04-20 14:34:10 PDT
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.
Comment 5 WebKit Review Bot 2010-04-20 16:02:25 PDT
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
Comment 6 James Robinson 2010-04-20 16:09:39 PDT
I have new chromium baselines ready to go whenever this relands, FYI.
Comment 7 Adam Barth 2010-04-20 16:18:59 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2010-04-20 20:01:17 PDT
(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.
Comment 9 Kenneth Rohde Christiansen 2010-04-21 09:50:37 PDT
Created attachment 53965 [details]
Patch with updated expected results
Comment 10 Kenneth Rohde Christiansen 2010-04-21 09:53:04 PDT
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.
Comment 11 Eric Seidel (no email) 2010-04-21 18:13:04 PDT
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.
Comment 12 Kenneth Rohde Christiansen 2010-04-21 20:50:40 PDT
(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 :-(
Comment 13 Kenneth Rohde Christiansen 2010-04-21 20:52:25 PDT
(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?
Comment 15 Kenneth Rohde Christiansen 2010-04-21 21:17:12 PDT
(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.
Comment 16 WebKit Commit Bot 2010-04-22 04:12:10 PDT
Comment on attachment 53965 [details]
Patch with updated expected results

Clearing flags on attachment: 53965

Committed r58085: <http://trac.webkit.org/changeset/58085>
Comment 17 WebKit Commit Bot 2010-04-22 04:12:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Kenneth Rohde Christiansen 2010-04-22 12:13:45 PDT
Created attachment 54083 [details]
Rename Media interface to StyleMedia

Second part of the change.
Comment 19 Kenneth Rohde Christiansen 2010-04-22 12:14:21 PDT
The spec also requires a rename of the interface itself.
Comment 20 Early Warning System Bot 2010-04-22 12:19:25 PDT
Attachment 54083 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1802055
Comment 21 WebKit Review Bot 2010-04-22 12:20:38 PDT
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.
Comment 22 Kenneth Rohde Christiansen 2010-04-22 12:22:25 PDT
(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
Comment 23 Kenneth Rohde Christiansen 2010-04-22 12:23:19 PDT
(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.
Comment 24 Kenneth Rohde Christiansen 2010-04-22 12:26:13 PDT
(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
Comment 25 Kenneth Rohde Christiansen 2010-04-22 12:38:00 PDT
Created attachment 54087 [details]
Patch
Comment 26 WebKit Review Bot 2010-04-22 12:43:08 PDT
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.
Comment 27 Early Warning System Bot 2010-04-22 12:46:04 PDT
Attachment 54087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1807043
Comment 28 Kenneth Rohde Christiansen 2010-04-22 13:28:41 PDT
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.
Comment 29 Kenneth Rohde Christiansen 2010-04-22 13:40:37 PDT
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.
Comment 30 WebKit Review Bot 2010-04-22 14:15:42 PDT
Attachment 54087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1880006
Comment 31 WebKit Review Bot 2010-04-22 16:09:57 PDT
Attachment 54087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1834043
Comment 32 Laszlo Gombos 2010-04-22 17:32:12 PDT
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.
Comment 33 Laszlo Gombos 2010-04-23 08:32:30 PDT
Comment on attachment 54087 [details]
Patch

As Kenneth promised to land it manually correctly.
Comment 34 David Kilzer (:ddkilzer) 2010-04-23 09:34:31 PDT
(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?!
Comment 37 Kenneth Rohde Christiansen 2010-04-23 10:14:15 PDT
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.
Comment 38 Kenneth Rohde Christiansen 2010-04-23 10:19:13 PDT
> 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.
Comment 39 Kenneth Rohde Christiansen 2010-04-23 10:22:09 PDT
(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.
Comment 40 David Kilzer (:ddkilzer) 2010-05-02 02:38:23 PDT
(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.
Comment 41 Kenneth Rohde Christiansen 2010-05-07 14:20:03 PDT
Please cherry-pick this one Simon.
Comment 42 Kenneth Rohde Christiansen 2010-05-07 14:21:57 PDT
(In reply to comment #41)
> Please cherry-pick this one Simon.

I mean Simon Hausmann. Sorry about the confusion Simon Fraser :-)
Comment 43 Simon Hausmann 2010-05-12 03:30:38 PDT
Revision r58085 cherry-picked into qtwebkit-2.0 with commit 5b263f2040886fc71100cb507d2e80f963021496
Revision r58168 cherry-picked into qtwebkit-2.0 with commit 35a9c2373f8260aab20d3f1b1fcb862fb0f35694