Fixing inconsistency with Media Query append/deleteMedium.

Fixing inconsistency between spec and Media Query CSSOM
methods appendMedium and deleteMedium.

Check the test for details, as it explain the relation
between the spec and the expected result.

Spec: http://dev.w3.org/csswg/cssom/#the-medialist-interface

The changes are mostly to corner cases, but it makes our
implementation more in line with Gecko and Presto, though
both has a few obvious inconsistencies with the spec.

R=apavlov@chromium.org, eseidel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151484

[apavlov, 2013-05-21 15:31:22]

Kenneth,

Did you mean to add reviewers to this patch and send mail?

https://chromiumcodereview.appspot.com/15094019/diff/8001/LayoutTests/fast/me...
File LayoutTests/fast/media/mq-js-stylesheet-media-03.html (right):

https://chromiumcodereview.appspot.com/15094019/diff/8001/LayoutTests/fast/me...
LayoutTests/fast/media/mq-js-stylesheet-media-03.html:28: <!-- FIXME: NOTE! This
will fail until dynamic setting of media property causes style recalculation.
I guess this comment should be removed as well?

[kenneth.christiansen, 2013-05-25 23:49:05]

On 2013/05/21 15:31:22, apavlov wrote:
> Kenneth,
> 
> Did you mean to add reviewers to this patch and send mail?

Not yet :-) I wanted to do better cross browser testing and ask some questions
to the spec author as it seems that the methods should not throw exceptions at
all, though both firefox and webkit does so.

I am currently travelling (in SFO) so I will not work on this before tuesday.

[kenneth.r.christiansen, 2013-05-29 14:36:28]

New patch up!

[apavlov, 2013-05-29 15:44:36]

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
File Source/core/css/MediaList.cpp (right):

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:120: return adoptPtr(new
MediaQuery(MediaQuery::None, medium, nullptr));
Should you add something like
// Fall through.
after this line?

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:125: default:
Should we guard ourselves against the default-case by ASSERT_NOT_REACHED() ?

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:163: // a media query list steps and return null
if more than one media query
Should the "parse a media query list" be quoted so that the sentence structure
would become a bit more clear? (I've been trying to attribute "a" to "steps",
which is wrong, while "parse a media query list" sounds like an algorithm name
from the respective spec.)

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:168: // If "null", terminate these steps.
Can it be more than 1, as opposed to 0?

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:189: // To parse a media query for a given string
means to follow the parse
Ditto for quoting

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:195: // If "null", terminate these steps.
Ditto for the comment

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:203: // comparing the media query with m returns
true.
What is "m"?

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
File Source/core/css/MediaList.h (right):

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.h:86: MediaQueryParserMode m_parserMode;
Should this occupy 2 bits, and m_lastLine (added by me, it seems) 30 bits?

[kenneth.r.christiansen, 2013-05-29 15:52:56]

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
File Source/core/css/MediaList.cpp (right):

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:120: return adoptPtr(new
MediaQuery(MediaQuery::None, medium, nullptr));
On 2013/05/29 15:44:36, apavlov wrote:
> Should you add something like
> // Fall through.
> after this line?

I actually had that :-) Let me readd that

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:125: default:
On 2013/05/29 15:44:36, apavlov wrote:
> Should we guard ourselves against the default-case by ASSERT_NOT_REACHED() ?

Fine with me

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:163: // a media query list steps and return null
if more than one media query
On 2013/05/29 15:44:36, apavlov wrote:
> Should the "parse a media query list" be quoted so that the sentence structure
> would become a bit more clear? (I've been trying to attribute "a" to "steps",
> which is wrong, while "parse a media query list" sounds like an algorithm name
> from the respective spec.)

Just quotes around that? That is fine with me.

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:168: // If "null", terminate these steps.
On 2013/05/29 15:44:36, apavlov wrote:
> Can it be more than 1, as opposed to 0?

No, it cannot according to the spec. It has to be exactly one. The spec
considers the result equal to "null" if it is not exacltly one

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:195: // If "null", terminate these steps.
On 2013/05/29 15:44:36, apavlov wrote:
> Ditto for the comment

Suggestions for the comments?

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.cpp:203: // comparing the media query with m returns
true.
On 2013/05/29 15:44:36, apavlov wrote:
> What is "m"?

Copy paste from the algorithm, forgot to remove the 'm'

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
File Source/core/css/MediaList.h (right):

https://chromiumcodereview.appspot.com/15094019/diff/15001/Source/core/css/Me...
Source/core/css/MediaList.h:86: MediaQueryParserMode m_parserMode;
On 2013/05/29 15:44:36, apavlov wrote:
> Should this occupy 2 bits, and m_lastLine (added by me, it seems) 30 bits?

Sounds fine. Let me do that change

[kenneth.r.christiansen, 2013-05-29 17:59:35]

Fixed!

[eseidel, 2013-05-29 23:12:11]

lgtm

[commit-bot: I haz the power, 2013-05-29 23:12:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/15094...

[eseidel, 2013-05-29 23:12:34]

Looks like you've been working with apavlov and he should lgtm as well.

[apavlov, 2013-05-30 09:49:56]

LGTM with a typo :)

https://chromiumcodereview.appspot.com/15094019/diff/27001/Source/core/css/Me...
File Source/core/css/MediaList.cpp (right):

https://chromiumcodereview.appspot.com/15094019/diff/27001/Source/core/css/Me...
Source/core/css/MediaList.cpp:121: // Fall throught.
typo: should be "through"

[commit-bot: I haz the power, 2013-05-30 11:14:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/15094...

[commit-bot: I haz the power, 2013-05-30 12:29:34]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-30 12:39:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/15094...

[commit-bot: I haz the power, 2013-05-30 13:55:24]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[kenneth.r.christiansen, 2013-05-30 14:23:05]

Committed patchset #8 manually as r151484 (presubmit successful).