Bug 36187 - Rename window.media to window.styleMedia
: Rename window.media to window.styleMedia
Status: CLOSED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 35784
  Show dependency treegraph
 
Reported: 2010-03-16 13:43 PST by
Modified: 2010-05-12 03:30 PST (History)


Attachments
Patch (7.34 KB, patch)
2010-04-20 13:21 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Patch with updated expected results (30.21 KB, patch)
2010-04-21 09:50 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Rename Media interface to StyleMedia (31.30 KB, patch)
2010-04-22 12:13 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.67 KB, patch)
2010-04-22 12:38 PST, Kenneth Rohde Christiansen
laszlo.gombos: review+
kenneth: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-16 13:43:25 PST
Anne changed the CSS OM View spec to use 'styleMedia':
http://dev.w3.org/csswg/cssom-view/
------- Comment #1 From 2010-04-20 13:21:59 PST -------
Created an attachment (id=53872) [details]
Patch
------- Comment #2 From 2010-04-20 13:22:55 PST -------
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 From 2010-04-20 13:26:39 PST -------
It seems that the class also changed name. I will do an additional patch to make that change.
------- Comment #4 From 2010-04-20 14:34:10 PST -------
(From update of attachment 53872 [details])
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 From 2010-04-20 16:02:25 PST -------
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 From 2010-04-20 16:09:39 PST -------
I have new chromium baselines ready to go whenever this relands, FYI.
------- Comment #7 From 2010-04-20 16:18:59 PST -------
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 From 2010-04-20 20:01:17 PST -------
(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 From 2010-04-21 09:50:37 PST -------
Created an attachment (id=53965) [details]
Patch with updated expected results
------- Comment #10 From 2010-04-21 09:53:04 PST -------
(From update of attachment 53965 [details])
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 From 2010-04-21 18:13:04 PST -------
(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.
------- Comment #12 From 2010-04-21 20:50:40 PST -------
(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 :-(
------- Comment #13 From 2010-04-21 20:52:25 PST -------
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 53872 [details] [details] [details])
> > Cleared Simon Fraser's review+ from obsolete attachment 53872 [details] [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 From 2010-04-21 21:17:12 PST -------
(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 From 2010-04-22 04:12:10 PST -------
(From update of attachment 53965 [details])
Clearing flags on attachment: 53965

Committed r58085: <http://trac.webkit.org/changeset/58085>
------- Comment #17 From 2010-04-22 04:12:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2010-04-22 12:13:45 PST -------
Created an attachment (id=54083) [details]
Rename Media interface to StyleMedia

Second part of the change.
------- Comment #19 From 2010-04-22 12:14:21 PST -------
The spec also requires a rename of the interface itself.
------- Comment #20 From 2010-04-22 12:19:25 PST -------
Attachment 54083 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1802055
------- Comment #21 From 2010-04-22 12:20:38 PST -------
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 From 2010-04-22 12:22:25 PST -------
(In reply to comment #21)
> Attachment 54083 [details] [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 From 2010-04-22 12:23:19 PST -------
(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.
------- Comment #24 From 2010-04-22 12:26:13 PST -------
(In reply to comment #23)
> (In reply to comment #20)
> > Attachment 54083 [details] [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 From 2010-04-22 12:38:00 PST -------
Created an attachment (id=54087) [details]
Patch
------- Comment #26 From 2010-04-22 12:43:08 PST -------
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 From 2010-04-22 12:46:04 PST -------
Attachment 54087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1807043
------- Comment #28 From 2010-04-22 13:28:41 PST -------
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 From 2010-04-22 13:40:37 PST -------
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 From 2010-04-22 14:15:42 PST -------
Attachment 54087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1880006
------- Comment #31 From 2010-04-22 16:09:57 PST -------
Attachment 54087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1834043
------- Comment #32 From 2010-04-22 17:32:12 PST -------
(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.
------- Comment #33 From 2010-04-23 08:32:30 PST -------
(From update of attachment 54087 [details])
As Kenneth promised to land it manually correctly.
------- Comment #34 From 2010-04-23 09:34:31 PST -------
(In reply to comment #32)
> (From update of attachment 54087 [details] [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] [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 From 2010-04-23 10:14:15 PST -------
(From update of attachment 54087 [details])
Landed in 58168 with lots of trouble.

Thanks for everyone helping me get the bots green as fast as possible.
------- Comment #38 From 2010-04-23 10:19:13 PST -------
> 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 From 2010-04-23 10:22:09 PST -------
(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 From 2010-05-02 02:38:23 PST -------
(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 From 2010-05-07 14:20:03 PST -------
Please cherry-pick this one Simon.
------- Comment #42 From 2010-05-07 14:21:57 PST -------
(In reply to comment #41)
> Please cherry-pick this one Simon.

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