Bug 37205 – Implement CSSOM View matchMedia interface

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Luiz Agostini]

To add support for ViewModeChangedEvent.

[Antonio Gomes]

it is always good to have more detailed bug description as well as:

* always categorizing it corrently (this one is a enhacement),
* using Qt keywork
* a use case for this feature is REQUIRED for this case I think
* a spec explaining that, if apropriate

if possible, pls do

[Kenneth Rohde Christiansen]

> * using Qt keywork

It is not Qt specific, but the blocker takes care of that.

> * a use case for this feature is REQUIRED for this case I think

The use case should be explained in the spec, which Luiz should have added a link to.

Btw, we have an action at the weekly W3C for discussing whether to change the name of the event to *Change* or leave it as *Changed*. I hope that will be figured out tomorrow.

For Qt only bugs please use http://webkit.org/new-qtwebkit-bug for creation.

[Luiz Agostini]

specification can be found at http://dev.w3.org/2006/waf/widgets-vm/vm-interfaces.src.html

[Luiz Agostini]

Created attachment 52987 [details]
patch 1

[Luiz Agostini]

Created attachment 53001 [details]
patch 2

[Luiz Agostini]

As the specification seems to be changing the implementation needs to wait. Specification independent part of the latest patch was moved to bug 37505 that is now blocking this bug.

[Luiz Agostini]

Created attachment 55394 [details]
patch 3

First draft. Main objective is to receive some feedback and to use this patch as a reference for discussion.

Setting r? to get some feedback on the idea and the implementation.

The consensus on the W3C blah-blah [1] mailing list was that instead of adding individual DOM events for changes to media features, we should instead make it possible to get notified when a user defined media query has changed.

The idea was making it possible to supply a JavaScript function to the styleMedia.matchMedium(...) function.

As no exact IDL was proposed, I came up with one myself which I think fits the use-case, and implemented the feature for WebKit.

The result of my work became the following IDL, for which I would like comments/feedback:

    interface MediaChangeListener {
        void mediaChanged(in boolean queryResult);
    };

    interface StyleMedia {
        readonly attribute DOMString type;
        boolean matchMedium(in DOMString mediaquery, in MediaChangeListener listener);
    };

If listener is supplied to the  matchMedium call then its mediaChanged method will be called every time when then result of the expression given in 'mediaquery' changes.
mediaChanged parameter queryResult receives the current value (true/false) of the corresponding mediaquery.

The idea and actual implementation has already gone through some initial feedback rounds with Kenneth Rohde Christiansen

[Simon Fraser (smfr)]

Comment on attachment 55394 [details]
patch 3

I don't like this idea. You're forcing the page author to call matchMedium() just to get callbacks about when media queries change. I think we need some new way in CSS to request that events are fired when media queries change, maybe via the CSS OM.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (From update of attachment 55394 [details])
> I don't like this idea. You're forcing the page author to call matchMedium()
> just to get callbacks about when media queries change. I think we need some new
> way in CSS to request that events are fired when media queries change, maybe
> via the CSS OM.

The idea came from dbaron and was discussed on irc with Anne (editor for CSSOM)

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (From update of attachment 55394 [details])
> I don't like this idea. You're forcing the page author to call matchMedium()
> just to get callbacks about when media queries change. I think we need some new
> way in CSS to request that events are fired when media queries change, maybe
> via the CSS OM.

Would you like the idea if there was a separate method for adding the listener? to avoid the first evaluation?

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

(In reply to comment #9)
> The idea came from dbaron and was discussed on irc with Anne (editor for CSSOM)

I think there needs to be some discussion on www-style

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> (In reply to comment #9)
> > The idea came from dbaron and was discussed on irc with Anne (editor for CSSOM)
> 
> I think there needs to be some discussion on www-style

OK, I will ask Luiz to bring the discussion there.

[Luiz Agostini]

(In reply to comment #7)
> Created an attachment (id=55394) [details]
> patch 3
> 
> First draft. Main objective is to receive some feedback and to use this patch
> as a reference for discussion.
> 
> Setting r? to get some feedback on the idea and the implementation.
> 
> The consensus on the W3C blah-blah [1] mailing list was that instead of adding
> individual DOM events for changes to media features, we should instead make it
> possible to get notified when a user defined media query has changed.
> 
> The idea was making it possible to supply a JavaScript function to the
> styleMedia.matchMedium(...) function.
> 
> As no exact IDL was proposed, I came up with one myself which I think fits the
> use-case, and implemented the feature for WebKit.
> 
> The result of my work became the following IDL, for which I would like
> comments/feedback:
> 
>     interface MediaChangeListener {
>         void mediaChanged(in boolean queryResult);
>     };
> 
>     interface StyleMedia {
>         readonly attribute DOMString type;
>         boolean matchMedium(in DOMString mediaquery, in MediaChangeListener
> listener);
>     };
> 
> If listener is supplied to the  matchMedium call then its mediaChanged method
> will be called every time when then result of the expression given in
> 'mediaquery' changes.
> mediaChanged parameter queryResult receives the current value (true/false) of
> the corresponding mediaquery.
> 
> The idea and actual implementation has already gone through some initial
> feedback rounds with Kenneth Rohde Christiansen

The actual mailing list is:

Extending CSSOM Views matchMedium with callback. [e-mail].
http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/0071.html

[Luiz Agostini]

(In reply to comment #8)
> (From update of attachment 55394 [details])
> I don't like this idea. You're forcing the page author to call matchMedium()
> just to get callbacks about when media queries change. I think we need some new
> way in CSS to request that events are fired when media queries change, maybe
> via the CSS OM.

Do you mean to trigger an event every time one of the media features changes?

Some advantages of the current approach:

1) if you don't subscribe to any media query, there is no overhead
2) the query itself is cached (MediaList object)
3) with a MediaFeatureChangeEvent, the user would have to implement something similar to what I implemented inside WebCore, with little change of optimization. 
4) the current implemtation is quite simple and can easily be optimized if it proves to be a bottleneck.

Some comment regarding implementation:

To know if a media query changed it is needed to keep its current value so it is needed to evaluate the query when the request for events is received anyway. It makes sense to just put it in matchMedium. 

Listener parameter is optional. If it is not supplied matchMedium behavior is just to return the result of the query.

It is needed to evaluate the queries to check if it has changed. So, as the current value is available at event time it is sent to the user.

[Simon Fraser (smfr)]

(In reply to comment #14)
> The actual mailing list is:
> 
> Extending CSSOM Views matchMedium with callback. [e-mail].
> http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/0071.html

This is general CSSOM feature, not a webapps feature, so www-style is the correct list.

Let's discuss there, and not in this bug.

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> (In reply to comment #14)
> > The actual mailing list is:
> > 
> > Extending CSSOM Views matchMedium with callback. [e-mail].
> > http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/0071.html
> 
> This is general CSSOM feature, not a webapps feature, so www-style is the
> correct list.
> 
> Let's discuss there, and not in this bug.

I think that it might make sense to cross post, as the idea comes from the fact that webapp developers needed to react on media features changes such as "(orientation: ...)", "(view-mode: ...)" etc.

[Luiz Agostini]

Created attachment 57489 [details]
test - trying to find help with bindings.

This patch is just a test.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/StyleMedia.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/css/StyleMedia.h:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/css/StyleMedia.cpp:117:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/css/StyleMedia.cpp:160:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 4 in 17 files


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

[Luiz Agostini]

Created attachment 57491 [details]
test - trying to find help with bindings.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/QueryCallback.h:20:  #ifndef header guard has wrong style, please use: QueryCallback_h  [build/header_guard] [5]
WebCore/css/QueryCallback.h:31:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/css/QueryCallback.h:33:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/css/QueryCallback.h:34:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 19 files


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

[WebKit Review Bot]

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

[Luiz Agostini]

Created attachment 57574 [details]
test

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

Attachment 57574 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2784062

[Kenneth Rohde Christiansen]

If the matchMedium etc are really going to be renamed, you need to push that on the mailing list. Microsoft have just added matchMedium to their IE9 preview.

If the change goes through, you should start by supplying a patch for the renaming of these existing methods.

[Darin Adler]

Comment on attachment 57574 [details]
test

Since the purpose of posting this patch was to get it tested, and it has been, setting review- now.

[Luiz Agostini]

Created attachment 64174 [details]
patch 4

[WebKit Review Bot]

Attachment 64174 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3728062

[WebKit Review Bot]

Attachment 64174 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3712073

[WebKit Review Bot]

Attachment 64174 [details] did not build on win:
Build output: http://queues.webkit.org/results/3752064

[Luiz Agostini]

Created attachment 64251 [details]
patch 5

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/v8/V8BooleanCallback.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 14 files


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

[WebKit Review Bot]

Attachment 64251 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3744093

[Luiz Agostini]

Created attachment 64631 [details]
patch 6

[Simon Fraser (smfr)]

The CSS-WG still hasn't decided on an API, so I'm not willing to review this yet.

[WebKit Review Bot]

Attachment 64631 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3738313

[WebKit Review Bot]

Attachment 64631 [details] did not build on win:
Build output: http://queues.webkit.org/results/3747291

[Kenneth Rohde Christiansen]

(In reply to comment #37)
> The CSS-WG still hasn't decided on an API, so I'm not willing to review this yet.

Is it somehow possible to let Luiz participate in our of your WG calls and discuss this?

[Anne van Kesteren]

Discussion happens here: http://lists.w3.org/Archives/Public/www-style/ No telephones involved :-)

[Simon Fraser (smfr)]

Most recent proposal:
http://lists.w3.org/Archives/Public/www-style/2010Aug/0343.html

[Luiz Agostini]

Created attachment 64934 [details]
implementing most recent specification

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSMediaQuery.cpp:128:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 30 files


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

[WebKit Review Bot]

Attachment 64934 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3756446

[Luiz Agostini]

Win EWS always fails to apply the patch if I make changes to WebCore.vcproj/WebCore.vcproj.
Is there any known workaround for this issue?

[Kenneth Rohde Christiansen]

Adding Eric, he might be able to answer your question

[Eric Seidel (no email)]

I believe the failure to apply is bug 43981.

[Kenneth Rohde Christiansen]

Can any of the GTK guys look into the GTK build issues? and test the patch locally? It probably needs some work due to your binding system.

[Kenneth Rohde Christiansen]

Simon, as the patch seems to be pretty final, do you feel like giving it a preliminary review?

[Adam Barth]

Comment on attachment 64934 [details]
implementing most recent specification

> WebCore/ChangeLog:23
> +        Preliminar tests in Qt and Safari.
Preliminar => Preliminary?

> WebCore/ChangeLog:27
> +        No new tests. (OOPS!)
This line will prevent this patch from landing.

[Luiz Agostini]

Created attachment 66502 [details]
patch

[Eric Seidel (no email)]

Attachment 66502 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3945086

[Adam Barth]

Comment on attachment 66502 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66502&action=prettypatch

I found this patch very mysterious.  The changes to CodeGeneratorJS don't seem right.  There's too much of an assumption that these objects either return bool or void.  Also, passing 1/0 to that help function isn't terribly enlightening.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:39
> +namespace WebCore {
> +
> +
Please remove blank line.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:40
> +static bool hasNoValue(const JSValue value)
const JSValue&    ?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:47
> +bool JSMediaQueryListListener::isEqual(MediaQueryListListener* other)
> +{
> +    JSMediaQueryListListener* ptr = static_cast<JSMediaQueryListListener*>(other);
How do we know this case is valid?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:52
> +    if (hasNoValue(m_data->callback()))
> +        return hasNoValue(ptr->m_data->callback());
Abstracting hasNoValue into its own function just obfuscates what's going on here.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:55
> +    ExecState* exec = m_data->globalObject()->globalExec();
> +    return JSValueIsEqual(toRef(exec), toRef(exec, m_data->callback()), toRef(exec, ptr->m_data->callback()), 0);
Generally, grabbing the exec state like this is a bad idea.  I don't understand yet what this function is for.  Hopefully I'll learn later in the patch.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2165
> +            push(@headerContent, "    virtual " . GetNativeTypeForCallbacks($function->signature->type, 0) . " " . $function->signature->name . "(");
Yuck.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2261
> +            if (GetNativeType($function->signature->type) eq "bool") {
> +                push(@implContent, "        return true;\n\n");
> +            } else {
> +                push(@implContent, "        return;\n\n");
> +            }
This seems wrong.  What if there are other types besides bool and void?

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2269
> +            foreach my $param (@params) {
> +                if (!($param->type eq "boolean")) {
> +                    push(@implContent, "    ExecState* exec = m_data->globalObject()->globalExec();\n");
> +                    last;
> +                }
> +            }
Why is grabbing the ExecState conditional on whether there's a boolean parameter?  What if there's a string parameter?  Would we not need an ExecState in that case?

> WebCore/bindings/scripts/CodeGeneratorV8.pm:2225
> -                push(@args, GetNativeTypeForCallbacks($param->type) . " " . $param->name);
> +                push(@args, GetNativeTypeForCallbacks($param->type, 1) . " " . $param->name);
Someone who reads this code later is going to have no idea what this 1 means.

> WebCore/css/MediaQueryList.cpp:39
> +PassRefPtr<MediaQueryList> MediaQueryList::create(PassRefPtr<MediaQueryVector> vector, PassRefPtr<MediaList> media, bool matches)
> +{
> +    return adoptRef(new MediaQueryList(vector, media, matches));
> +}
We usually put these in the header.

> WebCore/css/MediaQueryList.cpp:45
> +    , m_changeRound(m_evalRound - 1)
The -1 here is super mysterious.

> WebCore/css/MediaQueryListListener.idl:29
> +        [Custom] boolean isEqual(in MediaQueryListListener other);
I see.  Why do we have this method?  Isn't JavaScript's notion of equality sufficient?  Which spec is this from?

> WebCore/css/MediaQueryVector.cpp:103
> +    return new MediaQueryEvaluator(mediaType(), m_frame, rootStyle.get());
Naked new.  Please use adoptPtr.

[Luiz Agostini]

(In reply to comment #54)
> (From update of attachment 66502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66502&action=prettypatch
> 
> I found this patch very mysterious.  The changes to CodeGeneratorJS don't seem right.  There's too much of an assumption that these objects either return bool or void.  

The previous implementation was putting a COMPILE_ASSERT(false) in the generated header file if the return type was different of boolean. 
I just made a small change to accept void as well.

> Also, passing 1/0 to that help function isn't terribly enlightening.

That is boolean for Perl. Any suggestions?

> 
> > WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:39
> > +namespace WebCore {
> > +
> > +
> Please remove blank line.

ok

> 
> > WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:40
> > +static bool hasNoValue(const JSValue value)
> const JSValue&    ?

sure :) This function will be removed.
 
> > WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:47
> > +bool JSMediaQueryListListener::isEqual(MediaQueryListListener* other)
> > +{
> > +    JSMediaQueryListListener* ptr = static_cast<JSMediaQueryListListener*>(other);
> How do we know this case is valid?

I should use dynamic_cast instaed.

> 
> > WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:52
> > +    if (hasNoValue(m_data->callback()))
> > +        return hasNoValue(ptr->m_data->callback());
> Abstracting hasNoValue into its own function just obfuscates what's going on here.

ok

> 
> > WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:55
> > +    ExecState* exec = m_data->globalObject()->globalExec();
> > +    return JSValueIsEqual(toRef(exec), toRef(exec, m_data->callback()), toRef(exec, ptr->m_data->callback()), 0);
> Generally, grabbing the exec state like this is a bad idea.  I don't understand yet what this function is for.  Hopefully I'll learn later in the patch.

It is what is done when calling the listeners (see JSCallbackData::invokeCallback).
The function is for recognizing that a listener is been added more then once and to find which is the listener to remove in removeListener calls.
The problem is that, for example, different instances of class MediaQueryListListener may refer to the same Javascript function (a new MediaQueryListListener instance is created every time methods addListener and removeListener are called from javascript)

> 
> > WebCore/bindings/scripts/CodeGeneratorJS.pm:2165
> > +            push(@headerContent, "    virtual " . GetNativeTypeForCallbacks($function->signature->type, 0) . " " . $function->signature->name . "(");
> Yuck.
> 
> > WebCore/bindings/scripts/CodeGeneratorJS.pm:2261
> > +            if (GetNativeType($function->signature->type) eq "bool") {
> > +                push(@implContent, "        return true;\n\n");
> > +            } else {
> > +                push(@implContent, "        return;\n\n");
> > +            }
> This seems wrong.  What if there are other types besides bool and void?

There are no other possible types. Other types are ignored in this part of the code. 
Other types are explicitly rejected by putting a COMPILE_ASSERT(false) in the generated header.

> 
> > WebCore/bindings/scripts/CodeGeneratorJS.pm:2269
> > +            foreach my $param (@params) {
> > +                if (!($param->type eq "boolean")) {
> > +                    push(@implContent, "    ExecState* exec = m_data->globalObject()->globalExec();\n");
> > +                    last;
> > +                }
> > +            }
> Why is grabbing the ExecState conditional on whether there's a boolean parameter?  What if there's a string parameter?  Would we not need an ExecState in that case?

The parameter was boolean in previous specification but it changed for an interface.
The problem was that an unused exec variable would generate a warning that is treated as error in some builds.
As the parameter I am interested on is no longer boolean I will remove this part of the code.

> 
> > WebCore/bindings/scripts/CodeGeneratorV8.pm:2225
> > -                push(@args, GetNativeTypeForCallbacks($param->type) . " " . $param->name);
> > +                push(@args, GetNativeTypeForCallbacks($param->type, 1) . " " . $param->name);
> Someone who reads this code later is going to have no idea what this 1 means.

I understand. But there is no "true" or "false" in Perl and that is why I used 0 or 1.
How could I make it more clear? Should I create a variable with a meaningful name? I will do this.

> 
> > WebCore/css/MediaQueryList.cpp:39
> > +PassRefPtr<MediaQueryList> MediaQueryList::create(PassRefPtr<MediaQueryVector> vector, PassRefPtr<MediaList> media, bool matches)
> > +{
> > +    return adoptRef(new MediaQueryList(vector, media, matches));
> > +}
> We usually put these in the header.

ok

> 
> > WebCore/css/MediaQueryList.cpp:45
> > +    , m_changeRound(m_evalRound - 1)
> The -1 here is super mysterious.

It just make sure that initial values of m_changeRound and m_evalRound are different.

> 
> > WebCore/css/MediaQueryListListener.idl:29
> > +        [Custom] boolean isEqual(in MediaQueryListListener other);
> I see.  Why do we have this method?  Isn't JavaScript's notion of equality sufficient?  Which spec is this from?

We may not be talking about Javascript. But even if it was just about Javascript, how can I know if different instances of class MediaQueryListListener refer to the same Javascript function?
The specification is http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface

> 
> > WebCore/css/MediaQueryVector.cpp:103
> > +    return new MediaQueryEvaluator(mediaType(), m_frame, rootStyle.get());
> Naked new.  Please use adoptPtr.

sure :)

[Luiz Agostini]

Created attachment 66993 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 66993 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66993&action=prettypatch

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2265
> -            push(@implContent, "        return true;\n\n");
> +            if (GetNativeType($function->signature->type) eq "bool") {
> +                push(@implContent, "        return true;\n\n");
> +            } else {
> +                push(@implContent, "        return;\n\n");
> +            }
You need a comment here why this cannot be something else than bool and void.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2284
> -            push(@implContent, "    return !raisedException;\n");
> +            if (GetNativeType($function->signature->type) eq "bool") {
> +                push(@implContent, "    return !raisedException;\n");
> +            }
comment?

> WebCore/css/MediaQueryList.cpp:44
> +    , m_evalRound(m_vector->evalRound())
Couldn't a better name for than round be used?

> WebCore/css/MediaQueryList.cpp:45
> +    , m_changeRound(m_evalRound - 1)
There should be a comment about the -1

> WebCore/css/MediaQueryVector.cpp:154
> +        return;
> +    for (size_t i = 0; i < m_listeners.size(); ++i)
> +        m_listeners[i]->evaluate(evaluator.get());
Make a newline before this 'for'

> WebCore/page/DOMWindow.idl:775
> +
> +
> +        MediaQueryList matchMedia(in DOMString query);
Why two newlines here?

[Xan Lopez]

(In reply to comment #49)
> Can any of the GTK guys look into the GTK build issues? and test the patch locally? It probably needs some work due to your binding system.

If you are adding new IDL files that aren't optional you tell to tell the GObject bindings about it. Just add the new stuff following the pattern in WebCore/bindings/gobject/GNUmakefile.am

[Luiz Agostini]

Created attachment 67119 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 67119 [details]
patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> [phone]  Luiz Agostini  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Media listeners
> +        https://bugs.webkit.org/show_bug.cgi?id=37205

You should add some high-level comments here about what the new API does.


> +        MediaQueryListLsitener is a callback. Custom methods are needed to implement

Typo "MediaQueryListLsitener".



> diff --git a/WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp b/WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp


> +bool JSMediaQueryListListener::isEqualTo(MediaQueryListListener* other)
> +{
> +    if (!other || type() != other->type())
> +        return false;
> +
> +    JSMediaQueryListListener* ptr = static_cast<JSMediaQueryListListener*>(other);

Use a more descriptive name than "ptr".



> diff --git a/WebCore/css/MediaQueryList.cpp b/WebCore/css/MediaQueryList.cpp
> new file mode 100644
> index 0000000..9752a4f
> --- /dev/null
> +++ b/WebCore/css/MediaQueryList.cpp
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +#include "MediaQueryList.h"
> +
> +#include "MediaList.h"
> +#include "MediaQueryEvaluator.h"
> +#include "MediaQueryListListener.h"
> +#include "MediaQueryVector.h"
> +
> +namespace WebCore {
> +
> +PassRefPtr<MediaQueryList> MediaQueryList::create(PassRefPtr<MediaQueryVector> vector, PassRefPtr<MediaList> media, bool matches)
> +{
> +    return adoptRef(new MediaQueryList(vector, media, matches));
> +}
> +
> +MediaQueryList::MediaQueryList(PassRefPtr<MediaQueryVector> vector, PassRefPtr<MediaList> media, bool matches)
> +    : m_vector(vector)
> +    , m_media(media)
> +    , m_evalRound(m_vector->evalRound())
> +    , m_changeRound(m_evalRound - 1) // m_evalRound and m_changeRound initial values must be different
> +    , m_matches(matches)
> +{
> +}
> +
> +MediaQueryList::~MediaQueryList()
> +{
> +}
> +
> +String MediaQueryList::media() const
> +{
> +    return m_media->mediaText();
> +}
> +
> +bool MediaQueryList::changed() const
> +{
> +    return m_vector->evalRound() == m_changeRound;
> +}
> +
> +void MediaQueryList::addListener(PassRefPtr<MediaQueryListListener> listener)
> +{
> +    m_vector->addListener(listener, this);
> +}
> +
> +void MediaQueryList::removeListener(PassRefPtr<MediaQueryListListener> listener)
> +{
> +    m_vector->removeListener(listener, this);
> +}
> +
> +void MediaQueryList::evaluate(MediaQueryEvaluator* evaluator)
> +{
> +    if (m_evalRound == m_vector->evalRound() || !evaluator)
> +        return;
> +    m_evalRound = m_vector->evalRound();
> +
> +    bool newValue = evaluator->eval(m_media.get());
> +    if (newValue != m_matches) {
> +        m_matches = newValue;
> +        m_changeRound = m_evalRound;
> +    }
> +}
> +
> +}

> diff --git a/WebCore/css/MediaQueryList.h b/WebCore/css/MediaQueryList.h

> +class MediaQueryList : public RefCounted<MediaQueryList> {

I'd like to see a comment here describing what MediaQueryList is used for.

How does MediaQueryList differ from the existing MediaList, and your MediaQueryVector? We have lots of similarly named classes here, and it's too hard to keep them all distinct.

> +    int m_evalRound;
> +    int m_changeRound;

I'd like to see a comment here about what these mean.

> +    int m_matches;

or bool?

> diff --git a/WebCore/css/MediaQueryListListener.h b/WebCore/css/MediaQueryListListener.h


> +class MediaQueryList;
> +
> +class MediaQueryListListener : public RefCounted<MediaQueryListListener> {

Another good place for an explanatory comment.

> +public:
> +    enum Type {
> +        JSMediaQueryListListenerType,
> +        V8MediaQueryListListenerType
> +    };

What about native code types? Seems wrong to enumerate these here, but I don't know what the correct solution is.

> +    virtual ~MediaQueryListListener() {}
> +    virtual void queryChanged(MediaQueryList*) = 0;
> +    virtual int type() = 0;
> +    virtual bool isEqualTo(MediaQueryListListener* other) = 0;
> +
> +protected:
> +};

No need for 'protected'.

> diff --git a/WebCore/css/MediaQueryListListener.idl b/WebCore/css/MediaQueryListListener.idl

> +module view {
> +    interface [Callback] MediaQueryListListener {
> +        void queryChanged(in MediaQueryList mql);
> +        [Custom] long type();
> +        [Custom] boolean isEqualTo(in MediaQueryListListener other);
> +    };

I don't like the need to expose type(). We should talk to a bindings person to figure out how to avoid this.

> diff --git a/WebCore/css/MediaQueryVector.cpp b/WebCore/css/MediaQueryVector.cpp

> +MediaQueryListener::MediaQueryListener(PassRefPtr<MediaQueryListListener> listener, PassRefPtr<MediaQueryList> query)
> +    : m_listener(listener)
> +    , m_query(query)
> +{
> +}

MediaQueryListener should have its own file I think.

> +MediaQueryVector::MediaQueryVector(Frame* frame)

I don't think this should hold on to a frame; that may break when the page goes into the page cache.
Document would be better I think.

> +void MediaQueryVector::addListener(PassRefPtr<MediaQueryListListener> listener, PassRefPtr<MediaQueryList> query)
> +{
> +    if (m_terminated)
> +        return;
> +
> +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> +        if (m_listeners[i]->listener()->isEqualTo(listener.get()) && m_listeners[i]->query() == query.get())
> +            return;
> +    }

Would be more efficient to use a sorted hash on the canonical query string.

> diff --git a/WebCore/css/MediaQueryVector.h b/WebCore/css/MediaQueryVector.h

> +class MediaQueryListener {
> +public:
> +    MediaQueryListener(PassRefPtr<MediaQueryListListener>, PassRefPtr<MediaQueryList>);
> +    ~MediaQueryListener();
> +
> +    void evaluate(MediaQueryEvaluator*);
> +
> +    MediaQueryListListener* listener() { return m_listener.get(); }
> +    MediaQueryList* query() { return m_query.get(); }
> +
> +private:
> +    RefPtr<MediaQueryListListener> m_listener;
> +    RefPtr<MediaQueryList> m_query;
> +};
> +

> +class MediaQueryVector : public RefCounted<MediaQueryVector> {
Please add comment explaining the role of this class.

I don't like the name MediaQueryVector. This is a per-document singleton that handles all "matchMedia" queries, right?
So it should be MediaQueryMatcher or something.

> +private:
> +    MediaQueryVector(Frame*);
> +    PassOwnPtr<MediaQueryEvaluator> prepareEvaluator() const;
> +    String mediaType() const;
> +
> +    Frame* m_frame;
> +    Vector<MediaQueryListener*> m_listeners;

Naively, I'd expect this to have a hashtable that maps queries to listeners. Is there a reason you didn't implement it that way?

> +    int m_evalRound;

What does this mean?

> +    bool m_terminated;

Why is special termination handling required?

> diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp

> @@ -558,6 +561,16 @@ Document::~Document()
>          m_styleSheets->documentDestroyed();
>  
>      m_weakReference->clear();
> +
> +    if (m_mediaQueries)
> +        m_mediaQueries->terminate();

Why doesn't this just cause m_mediaQueries to be destroyed, and why can't m_mediaQueries be an OwnPtr?

> diff --git a/WebCore/page/DOMWindow.h b/WebCore/page/DOMWindow.h


> diff --git a/WebCore/page/DOMWindow.idl b/WebCore/page/DOMWindow.idl

> +        // CSSOM View Module
> +        MediaQueryList matchMedia(in DOMString query);

You should put it up next to styleMedia, and add a comment that styleMedia is deprecated.

[Luiz Agostini]

Created attachment 67202 [details]
patch

[Luiz Agostini]

(In reply to comment #60)

>
> How does MediaQueryList differ from the existing MediaList, and your MediaQueryVector?

MediaList is the object produced by parsing a media query list string and MediaQueryList is the object returned by window.matchMedia.


> We have lots of similarly named classes here, and it's too hard to keep them all distinct.

I agree. Maybe we could have a prefix for the classes that are used by the parser. It could be for example CSSMediaQuery, CSSMediaList, CSSMediaQueryExp, ...
I could work on it in a future patch.

>
> > +public:
> > +    enum Type {
> > +        JSMediaQueryListListenerType,
> > +        V8MediaQueryListListenerType
> > +    };
>
> What about native code types? Seems wrong to enumerate these here, but I don't know what the correct solution is.
>
> > diff --git a/WebCore/css/MediaQueryListListener.idl b/WebCore/css/MediaQueryListListener.idl
>
> > +module view {
> > +    interface [Callback] MediaQueryListListener {
> > +        void queryChanged(in MediaQueryList mql);
> > +        [Custom] long type();
> > +        [Custom] boolean isEqualTo(in MediaQueryListListener other);
> > +    };
>
> I don't like the need to expose type(). We should talk to a bindings person to figure out how to avoid this.

I am getting convinced that I will need to implement all the bindings by hand instead of reling on code generators, following EventListener example.
But it would be good I could do it in a future patch. For now javascript is working (JS and V8).

Feedback from bindings persons would be very welcome.


> MediaQueryListener should have its own file I think.

It is just a helper class. I made it an inner class of class MediaQueryMatcher


> > +void MediaQueryVector::addListener(PassRefPtr<MediaQueryListListener> listener, PassRefPtr<MediaQueryList> query)
> > +{
> > +    if (m_terminated)
> > +        return;
> > +
> > +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> > +        if (m_listeners[i]->listener()->isEqualTo(listener.get()) && m_listeners[i]->query() == query.get())
> > +            return;
> > +    }
>
> Would be more efficient to use a sorted hash on the canonical query string.

The canonical query string is not good here because different calls to window.matchMedia will produce different instances of MediaQueryList objects even if the same string is used.
The objective is to relate MediaQueryList objects and MediaQueryListListener objects and to preserve the order in which they have been added to be able to call the listeners in that same order.
I also do not want to have RefPtr<MediaQueryList> in any place other then in this vector's items.


>
> > +private:
> > +    MediaQueryVector(Frame*);
> > +    PassOwnPtr<MediaQueryEvaluator> prepareEvaluator() const;
> > +    String mediaType() const;
> > +
> > +    Frame* m_frame;
> > +    Vector<MediaQueryListener*> m_listeners;
>
> Naively, I'd expect this to have a hashtable that maps queries to listeners. Is there a reason you didn't implement it that way?
>
> > +    bool m_terminated;
>
> Why is special termination handling required?
>
> > diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
>
> > @@ -558,6 +561,16 @@ Document::~Document()
> >          m_styleSheets->documentDestroyed();
> >
> >      m_weakReference->clear();
> > +
> > +    if (m_mediaQueries)
> > +        m_mediaQueries->terminate();
>
> Why doesn't this just cause m_mediaQueries to be destroyed, and why can't m_mediaQueries be an OwnPtr?
>

I will try to explain the objects lifecycle as it is implemented:


                                                         MediaQueryListListener
                                                                    ^
                                                                    |
                                                                    |
     Document -------->  MediaQueryMatcher ------------------>  Listener
                                 ^                           *      |
                                 |                                  |
                                 |                                  |
                                 +------  MediaQueryList  <---------+
                                                ^
                                                |
                                                |
                                         JSMediaQueryList

* All arrows in this diagram, except the one between MediaQueryMatcher and Listener,
  are RefPtr<>.


No reference to MediaQueryList instances is kept by class MediaQueryMatcher itself. The reason
is that if MediaQueryList::addListener is not called the only reference to the MediaQueryList
object will be in Javascript and it will be destroyed as soon as the JS object is garbage
collected.

Class MediaQueryList does not keep a list of MediaQueryListListener. It calls
MediaQueryMatcher::addListener every time its addListener method is called, passing the
received MediaQueryListListener object and itself. MediaQueryMatcher::addListener will
then append a new MediaQueryMatcher::Listener to its internal vector, if the pair
MediaQueryListListener x MediaQueryList is not duplicated.

This new Listener object has a reference to the MediaQueryList object, that will keep
it alive to be passed to the callback. If all listeners are removed from that
MediaQueryList object, all Listener instances pointing to that MediaQueryList object will be
destroyed and again the only reference to the query will be in JS.

It may be that a MediaQueryMatcher instance has one or more Listener instances pointing to a
MediaQueryList and this MediaQueryList has a reference to that same MediaQueryMatcher instance,
preventing them from been destroyed (the cycle in the diagram). That is why the
MediaQueryMatcher object cannot be owned by the Document instance and an special termination
handling is required.

Method MediaQueryMatcher::terminate, called by Document just before releasing the
MediaQueryMatcher reference, will remove all references to MediaQueryList objects in
MediaQueryMatcher and will guarantee that no new one will be created. The only references to
MediaQueryList objects will be again in JS. All MediaQueryList objects will be destroyed as soon
as their corresponding JS objects get garbage collected and MediaQueryMatcher will die as soon
as the last MediaQueryList object dies.

[Simon Fraser (smfr)]

(In reply to comment #62)

> I agree. Maybe we could have a prefix for the classes that are used by the parser. It could be for example CSSMediaQuery, CSSMediaList, CSSMediaQueryExp, ...
> I could work on it in a future patch.

Or do it first!

> I am getting convinced that I will need to implement all the bindings by hand instead of reling on code generators, following EventListener example.
> But it would be good I could do it in a future patch. For now javascript is working (JS and V8).

I don't like incurring technical debt like this. Is it much work to do this from the start?

> > MediaQueryListener should have its own file I think.
> 
> It is just a helper class. I made it an inner class of class MediaQueryMatcher

OK

> I will try to explain the objects lifecycle as it is implemented:
> 
> 
>                                                          MediaQueryListListener
>                                                                     ^
>                                                                     |
>                                                                     |
>      Document -------->  MediaQueryMatcher ------------------>  Listener
>                                  ^                           *      |
>                                  |                                  |
>                                  |                                  |
>                                  +------  MediaQueryList  <---------+
>                                                 ^
>                                                 |
>                                                 |
>                                          JSMediaQueryList
> 
> * All arrows in this diagram, except the one between MediaQueryMatcher and Listener,
>   are RefPtr<>.
> 
> 
> No reference to MediaQueryList instances is kept by class MediaQueryMatcher itself. The reason
> is that if MediaQueryList::addListener is not called the only reference to the MediaQueryList
> object will be in Javascript and it will be destroyed as soon as the JS object is garbage
> collected.
> 
> Class MediaQueryList does not keep a list of MediaQueryListListener. It calls
> MediaQueryMatcher::addListener every time its addListener method is called, passing the
> received MediaQueryListListener object and itself. MediaQueryMatcher::addListener will
> then append a new MediaQueryMatcher::Listener to its internal vector, if the pair
> MediaQueryListListener x MediaQueryList is not duplicated.
> 
> This new Listener object has a reference to the MediaQueryList object, that will keep
> it alive to be passed to the callback. If all listeners are removed from that
> MediaQueryList object, all Listener instances pointing to that MediaQueryList object will be
> destroyed and again the only reference to the query will be in JS.
> 
> It may be that a MediaQueryMatcher instance has one or more Listener instances pointing to a
> MediaQueryList and this MediaQueryList has a reference to that same MediaQueryMatcher instance,
> preventing them from been destroyed (the cycle in the diagram). That is why the
> MediaQueryMatcher object cannot be owned by the Document instance and an special termination
> handling is required.
> 
> Method MediaQueryMatcher::terminate, called by Document just before releasing the
> MediaQueryMatcher reference, will remove all references to MediaQueryList objects in
> MediaQueryMatcher and will guarantee that no new one will be created. The only references to
> MediaQueryList objects will be again in JS. All MediaQueryList objects will be destroyed as soon
> as their corresponding JS objects get garbage collected and MediaQueryMatcher will die as soon
> as the last MediaQueryList object dies.

This would be great as a comment in the code.

[Luiz Agostini]

(In reply to comment #63)
> (In reply to comment #62)
> 
> > I agree. Maybe we could have a prefix for the classes that are used by the parser. It could be for example CSSMediaQuery, CSSMediaList, CSSMediaQueryExp, ...
> > I could work on it in a future patch.
> 
> Or do it first!

Just let me keep focus on this implementation for now :-). I may do it soon.

> 
> > I am getting convinced that I will need to implement all the bindings by hand instead of reling on code generators, following EventListener example.
> > But it would be good I could do it in a future patch. For now javascript is working (JS and V8).
> 
> I don't like incurring technical debt like this. Is it much work to do this from the start?

I would start working on it immediately. It is just about splitting the patch. I agree that it must be done now but it would be nice to work in smaller patches.
And this latest patch seems to be quite self contained.

> 
> > > MediaQueryListener should have its own file I think.
> > 
> > It is just a helper class. I made it an inner class of class MediaQueryMatcher
> 
> OK
> 
> > I will try to explain the objects lifecycle as it is implemented:
> > 
> > 
> >                                                          MediaQueryListListener
> >                                                                     ^
> >                                                                     |
> >                                                                     |
> >      Document -------->  MediaQueryMatcher ------------------>  Listener
> >                                  ^                           *      |
> >                                  |                                  |
> >                                  |                                  |
> >                                  +------  MediaQueryList  <---------+
> >                                                 ^
> >                                                 |
> >                                                 |
> >                                          JSMediaQueryList
> > 
> > * All arrows in this diagram, except the one between MediaQueryMatcher and Listener,
> >   are RefPtr<>.
> > 
> > 
> > No reference to MediaQueryList instances is kept by class MediaQueryMatcher itself. The reason
> > is that if MediaQueryList::addListener is not called the only reference to the MediaQueryList
> > object will be in Javascript and it will be destroyed as soon as the JS object is garbage
> > collected.
> > 
> > Class MediaQueryList does not keep a list of MediaQueryListListener. It calls
> > MediaQueryMatcher::addListener every time its addListener method is called, passing the
> > received MediaQueryListListener object and itself. MediaQueryMatcher::addListener will
> > then append a new MediaQueryMatcher::Listener to its internal vector, if the pair
> > MediaQueryListListener x MediaQueryList is not duplicated.
> > 
> > This new Listener object has a reference to the MediaQueryList object, that will keep
> > it alive to be passed to the callback. If all listeners are removed from that
> > MediaQueryList object, all Listener instances pointing to that MediaQueryList object will be
> > destroyed and again the only reference to the query will be in JS.
> > 
> > It may be that a MediaQueryMatcher instance has one or more Listener instances pointing to a
> > MediaQueryList and this MediaQueryList has a reference to that same MediaQueryMatcher instance,
> > preventing them from been destroyed (the cycle in the diagram). That is why the
> > MediaQueryMatcher object cannot be owned by the Document instance and an special termination
> > handling is required.
> > 
> > Method MediaQueryMatcher::terminate, called by Document just before releasing the
> > MediaQueryMatcher reference, will remove all references to MediaQueryList objects in
> > MediaQueryMatcher and will guarantee that no new one will be created. The only references to
> > MediaQueryList objects will be again in JS. All MediaQueryList objects will be destroyed as soon
> > as their corresponding JS objects get garbage collected and MediaQueryMatcher will die as soon
> > as the last MediaQueryList object dies.
> 
> This would be great as a comment in the code.

No problem. I will put it in MediaQueryMatcher.h.

[Luiz Agostini]

Created attachment 67227 [details]
patch

windows build system.
Some more comments in source code.

[Luiz Agostini]

Darin, would you mind taking a look?

[Darin Adler]

Comment on attachment 67227 [details]
patch

The patch to WebCore.vcproj failed to apply on the Windows EWS bot so this probably won’t work on Windows. Review coming shortly.

[Darin Adler]

Comment on attachment 67227 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67227&action=prettypatch

Generally looks ambitious and pretty good. But the V8/JS part of this is taking the wrong approach. An approach we are not taking anywhere else in WebKit that is pretty heavyweight. Please look at ScriptFunctionCall and such.

> LayoutTests/platform/chromium/test_expectations.txt:2498
> +BUG40680 SKIP : fast/media/media-query-list-02.html = FAIL
> +BUG40680 SKIP : fast/media/media-query-list-03.html = FAIL
I don’t get it. Why are we skipping these tests at the same time we are adding them?

> LayoutTests/platform/mac/Skipped:146
> +fast/media/media-query-list-02.html
> +fast/media/media-query-list-03.html
Same question.

> LayoutTests/platform/win/Skipped:854
> +fast/media/media-query-list-02.html
> +fast/media/media-query-list-03.html
Here too. Can we make these work instead of skipping them?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:30
> +#include <JavaScriptCore/APICast.h>
> +#include <JavaScriptCore/JSValueRef.h>
It’s wrong to be including this here. WebCore doesn’t use the public JavaScriptCore API.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:55
> +    return JSValueIsEqual(toRef(exec), toRef(exec, m_data->callback()), toRef(exec, jsListener->m_data->callback()), 0);
This should use JSValue::equal if weu’re trying to get the same result as JavaScript "==". But is that really the right behavior of this? I think this is strange behavior. Unprecedented even. Also, maybe we want "===" instead of "=="?

I also don’t see why we need a special case for the null JSValue. Very surprising. Is there a test case that covers this?

> WebCore/css/MediaQueryList.cpp:64
> +void MediaQueryList::removeListener(PassRefPtr<MediaQueryListListener> listener)
A remove function usually should not take a PassRefPtr. A raw pointer would make more sense.

> WebCore/css/MediaQueryList.cpp:82
> +    m_evalRound = m_matcher->evalRound();
> +    if (newValue != m_matches) {
> +        m_matches = newValue;
> +        m_changeRound = m_evalRound;
> +    }
The usual WebKit pattern is early return rather than nesting the work inside an if statement.

> WebCore/css/MediaQueryList.h:41
> +/**
We don’t use that "**" thing.

> WebCore/css/MediaQueryList.h:43
> +  The objects of this class are retuned by window.matchMedia. They may be used to
Typo: "retuned".

> WebCore/css/MediaQueryList.h:52
> +    static PassRefPtr<MediaQueryList> create(PassRefPtr<MediaQueryMatcher>, PassRefPtr<MediaList>, bool);
> +    ~MediaQueryList();
> +    String media() const;
> +    bool matches();
Need a blank line here after the destructor.

Why isn’t matches a cost?

> WebCore/css/MediaQueryList.h:58
> +    bool changed();
This is a very confusing function name. What changed? Changed since when? Should this be const?

> WebCore/css/MediaQueryListListener.h:36
> +  MediaQueryListListener is an abstraction of the callbacks.
No idea what that means.

But also you should not have to have two different types of listener at runtime. We don’t have builds that mix JavaScript and V8. We do not want to use virtual functions just to get to the code from different JavaScript engines.

> WebCore/css/MediaQueryMatcher.cpp:133
> +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> +        if (m_listeners[i]->listener()->isEqualTo(listener.get()) && m_listeners[i]->query() == query.get())
> +            return;
> +    }
Is this really the needed behavior? Is a listener considered the same based on the JavaScript language "==" operator?

I think the way we’re doing the abstraction of the different script engines here is not good. Given that the need to accommodate both script engines is needed the pattern of classes like ScriptValue should be used, rather than having a custom runtime abstraction here.

> WebCore/css/MediaQueryMatcher.h:101
> +    static PassRefPtr<MediaQueryMatcher> create(Document* doc) { return adoptRef(new MediaQueryMatcher(doc)); }
Please don’t abbreviate document as "doc".

> WebCore/css/MediaQueryMatcher.h:110
> +    int evalRound() const { return m_evalRound; }
There has to be a better name for this. I don’t think “eval round” is a good term for it.

> WebCore/css/MediaQueryMatcher.h:130
> +private:
No reason to repeat private: here.

> WebCore/dom/Document.h:1319
> +    RefPtr<MediaQueryMatcher> m_mediaQueryMatcher;
Does this need to be owned by each document? Can it instead be global or per-Page?

[Luiz Agostini]

(In reply to comment #68)
> (From update of attachment 67227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67227&action=prettypatch
> 
> Generally looks ambitious and pretty good. But the V8/JS part of this is taking the wrong approach. An approach we are not taking anywhere else in WebKit that is pretty heavyweight. Please look at ScriptFunctionCall and such.

Could you please take a look at bug 43216. It is a preparation to the approach that you are suggesting, if I understood it. I will set review flag on and CC you.

> 
> > LayoutTests/platform/chromium/test_expectations.txt:2498
> > +BUG40680 SKIP : fast/media/media-query-list-02.html = FAIL
> > +BUG40680 SKIP : fast/media/media-query-list-03.html = FAIL
> I don’t get it. Why are we skipping these tests at the same time we are adding them?
> 
> > LayoutTests/platform/mac/Skipped:146
> > +fast/media/media-query-list-02.html
> > +fast/media/media-query-list-03.html
> Same question.
> 
> > LayoutTests/platform/win/Skipped:854
> > +fast/media/media-query-list-02.html
> > +fast/media/media-query-list-03.html
> Here too. Can we make these work instead of skipping them?

These tests are skipped here because they depend on method setViewModeMediaFeature() of LayoutTestController. It has been introduced in bug 37505 and I filed bugs to all main platforms (bug 43277, bug 43279 and bug 43280).
Today it is supported only by Qt and Gtk.

> > WebCore/css/MediaQueryList.cpp:64
> > +void MediaQueryList::removeListener(PassRefPtr<MediaQueryListListener> listener)
> A remove function usually should not take a PassRefPtr. A raw pointer would make more sense.

This is because code generator will pass a PassRefPtr.

> Why isn’t matches a cost?
> 
> > WebCore/css/MediaQueryList.h:58
> > +    bool changed();
> This is a very confusing function name. What changed? Changed since when? Should this be const?

These methods are not const because they try to evaluated the query again if it is outdated. For them to be const I would need to make some fields mutable. Should I do it?

> But also you should not have to have two different types of listener at runtime. We don’t have builds that mix JavaScript and V8. We do not want to use virtual functions just to get to the code from different JavaScript engines.

Yes, but if I understood I may have JS and ObjC objects simultaneously at runtime. In next patch I plan to follow your suggestions regarding JS/V8 and to try to add ObjC listeners.

> 
> > WebCore/css/MediaQueryMatcher.cpp:133
> > +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> > +        if (m_listeners[i]->listener()->isEqualTo(listener.get()) && m_listeners[i]->query() == query.get())
> > +            return;
> > +    }
> Is this really the needed behavior? Is a listener considered the same based on the JavaScript language "==" operator?

Yes. That is the idea.

> 
> I think the way we’re doing the abstraction of the different script engines here is not good. Given that the need to accommodate both script engines is needed the pattern of classes like ScriptValue should be used, rather than having a custom runtime abstraction here.

It is about ObjC and GObject listeners.

> 
> > WebCore/dom/Document.h:1319
> > +    RefPtr<MediaQueryMatcher> m_mediaQueryMatcher;
> Does this need to be owned by each document? Can it instead be global or per-Page?

Yes, I believe that it needs to be owned by the document.


I plan to submit a patch with a complete different bindings implementation soon. I hope I understood your comments regarding binding.

[Luiz Agostini]

Created attachment 67971 [details]
patch

[Luiz Agostini]

I did not set the review flag because of the dependency on bug 43216. All bots would fail.

It seems that it is enough to implement JS bindings at the end.

Darin, I kept the basic abstract class for the listeners because I believe that other bindings may be implemented in future. Am I right or should I remove the abstraction to avoid virtual calls anyway? I tried to follow your suggestions in any other cases.

[Luiz Agostini]

Created attachment 68456 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 68456 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68456&action=review

> WebCore/WebCore.pri:90
> +#    css/MediaQueryListListener.idl \

Why this? No comment?

> WebCore/css/MediaQueryList.cpp:45
> +    , m_changeRound(m_evalRound - 1) // m_evalRound and m_changeRound initial values must be different

Comments should end with a . (dot) :-)

> WebCore/css/MediaQueryList.h:67
> +    int m_evalRound; // indicates if the query has been evaluated after the last style selector change.
> +    int m_changeRound; // used to know if the query has changed in the last style selector change.

And start with capital letter...

> WebCore/css/MediaQueryListListener.h:53
> +    int type() const { return m_type; }

int?

> WebCore/css/MediaQueryMatcher.h:138
> +    int m_evaluationRound; // this value is incremented at style selector changes.

This*

> WebCore/css/MediaQueryMatcher.h:139
> +                           // it is used to avoid evaluating queries more then once

It*

> WebCore/page/DOMWindow.idl:144
> +        readonly attribute StyleMedia styleMedia; // deprecated

Why not remove it? or add a FIXME, like // FIXME: Deprecated.

[Darin Adler]

Sorry I did not have a chance to review yet.

I do not think the abstract base class and use of polymorphism is a good idea. We should not do that kind of thing just because we think we might need it in the future.

[Luiz Agostini]

Created attachment 69310 [details]
patch

Removing listener abstract base class.

[Luiz Agostini]

Darin, would you mind taking a look if you have time?

[Darin Adler]

Comment on attachment 69310 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69310&action=review

Looks good. Still some work to be done I think.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:289
> +    if ($type eq "MediaQueryListListener") {
> +        $implIncludes{"MediaQueryListListener.h"} = 1;
> +    }

It’s really disappointing that this is necessary. Adding a new type to the bindings generator is a lot of extra work.

Specifically, you need to add test cases to the bindings tests. Take a look at the run-bindings-tests script, the test cases in WebCore/bindings/scripts/test, and the expected test results in the subdirectories of that directory. You’ll need to add test cases, and update the expected test results.

> WebCore/css/MediaQueryList.cpp:44
> +    , m_evalRound(m_matcher->evaluationRound())

If the full name is evaluationRound, then I suggest m_evaluationRound rather than m_evalRound.

> WebCore/css/MediaQueryList.h:46
> +/*
> +  MediaQueryList interface is specified at http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface
> +  The objects of this class are returned by window.matchMedia. They may be used to
> +  retrieve the current value of the given media query and to add/remove listeners that
> +  will be called whenever the value of the query changes.
> +*/

In the WebKit project we normally use // comments, not /* */ comments, except for the license comment.

> WebCore/css/MediaQueryList.h:50
> +    ~MediaQueryList();

Why do we need to explicitly define the destructor? Did you try with the default constructor.

> WebCore/css/MediaQueryListListener.cpp:33
> +#if USE(JSC)
> +#include "JSMediaQueryList.h"
> +#else
> +#include "V8MediaQueryList.h"
> +#endif

Includes inside an #if normally go in their own paragraph separate from the main one.

> WebCore/css/MediaQueryListListener.cpp:53
> +bool MediaQueryListListener::isEqualTo(ScriptState* state, const MediaQueryListListener* other)
> +{
> +    return other && other->m_value.isEqual(state, m_value);
> +}

The use of JavaScript equality to define listener equality is extremely unusual; other listeners use object identity, not object equality, which is both more efficient and never involves running arbitrary JavaScript, while isEqualTo can involve running script. We need test cases covering this, and clarifying that it’s equality and not strict equality. I don’t see coverage in the tests you added.

> WebCore/css/MediaQueryListListener.h:41
> +/*
> +  See http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface
> +*/

Comment should be "//" style.

> WebCore/css/MediaQueryListListener.h:52
> +    MediaQueryListListener(ScriptValue value) : m_value(value) {}

WebKit style is to use a space inside the {}.

> WebCore/css/MediaQueryListListener.idl:33
> +        void queryChanged(in MediaQueryList mql);

Please use the word “list” here rather than “mql”; we use words rather than acronyms when possible.

> WebCore/css/MediaQueryMatcher.cpp:138
> +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> +        if (m_listeners[i]->listener()->isEqualTo(exec, listener.get()) && m_listeners[i]->query() == query.get())
> +            return;
> +    }

This code can reenter because a JavaScript equality comparison can involve running arbitrary script. We need to add some test cases covering that. If we do this wrong we could end up adding a security vulnerability. I doubt very much that the JavaScript equality rule is a good one.

> WebCore/css/MediaQueryMatcher.h:100
> +/*
> +  MediaQueryMatcher class is responsible for keeping a vector of pairs
> +  MediaQueryList x MediaQueryListListener. It is responsible for evaluating the queries
> +  whenever it is needed and to call the listeners if the corresponding query has changed.
> +  The listeners must be called in the very same order in which they have been added.
> +
> +  Object diagram:
> +
> +
> +                                                         MediaQueryListListener
> +                                                                    ^
> +                                                                    |
> +                                                                    |
> +     Document -------->  MediaQueryMatcher ------------------>  Listener
> +                                 ^                           *      |
> +                                 |                                  |
> +                                 |                                  |
> +                                 +------  MediaQueryList  <---------+
> +                                                ^
> +                                                |
> +                                                |
> +                                         JSMediaQueryList
> +
> +  * All arrows in this diagram, except the one between MediaQueryMatcher and Listener,
> +    are RefPtr<>.
> +
> +  Objects lifecycle:
> +
> +  No reference to MediaQueryList instances is kept by class MediaQueryMatcher itself. The reason
> +  is that if MediaQueryList::addListener is not called the only reference to the MediaQueryList
> +  object will be in Javascript and it will be destroyed as soon as the JS object is garbage
> +  collected.
> +
> +  Class MediaQueryList does not keep a list of MediaQueryListListener. It calls
> +  MediaQueryMatcher::addListener every time its addListener method is called, passing the
> +  received MediaQueryListListener object and itself. MediaQueryMatcher::addListener will
> +  then append a new MediaQueryMatcher::Listener to its internal vector, if the pair
> +  MediaQueryListListener x MediaQueryList is not duplicated.
> +
> +  This new Listener object has a reference to the MediaQueryList object, that will keep
> +  it alive to be passed to the callback. If all listeners are removed from that
> +  MediaQueryList object, all Listener instances pointing to that MediaQueryList object will be
> +  destroyed and again the only reference to the query will be in JS.
> +
> +  It may be that a MediaQueryMatcher instance has one or more Listener instances pointing to a
> +  MediaQueryList and this MediaQueryList has a reference to that same MediaQueryMatcher instance,
> +  preventing them from been destroyed (the cycle in the diagram). That is why the
> +  MediaQueryMatcher object cannot be owned by the Document instance and an special termination
> +  handling is required.
> +
> +  Method MediaQueryMatcher::terminate, called by Document just before releasing the
> +  MediaQueryMatcher reference, will remove all references to MediaQueryList objects in
> +  MediaQueryMatcher and will guarantee that no new one will be created. The only references to
> +  MediaQueryList objects will be again in JS. All MediaQueryList objects will be destroyed as soon
> +  as their corresponding JS objects get garbage collected and MediaQueryMatcher will die as soon
> +  as the last MediaQueryList object dies.
> +*/

There are many who would applaud you for writing extensive documentation. I am concerned that this is a long explanation which may quickly get out of sync with the code. A lot of this is worded in a way that I find confusing, with hypothetical situations that I don’t completely understand.

> WebCore/css/MediaQueryMatcher.h:104
> +    ~MediaQueryMatcher();

Once you change m_listeners, you probably won’t need an explicit destructor declaration.

> WebCore/css/MediaQueryMatcher.h:120
> +        ~Listener();

You don’t need this explicit destructor declaration.

> WebCore/css/MediaQueryMatcher.h:137
> +    Vector<Listener*> m_listeners;

This should be a Vector<OwnPtr<Listener> >. Making it use that type will allow you to remove much of the code.

> WebCore/css/MediaQueryMatcher.h:140
> +                           // and to avoid notifing a change more then once.

This long comment is indented in a way we normally don’t do. Also, I don’t know what “notifing a change” means.

> WebCore/dom/Document.cpp:572
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->terminate();

I would call this function documentDestroyed rather than terminate.

> WebCore/dom/Document.cpp:2855
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->evaluate();

This doesn’t seem right to me. I don’t understand why the operation is named evaluate, nor why styleSelectorChanged is the right time to do it. It seems to me we don’t want to do any new evaluation until someone calls matchMedia.

> WebCore/dom/Document.h:203
> +    PassRefPtr<MediaQueryList> matchMedia(const String&);

I think the document class should just have an accessor to return the media query matcher object. There’s no need to have the matchMedia forwarding function here. The code in DOMWindow can handle that part.

> WebCore/page/DOMWindow.idl:144
> +        readonly attribute StyleMedia styleMedia; // FIXME: deprecated

This is a confusing comment. Please don’t write that in the .idl file. If you are saying FIXME, then I presume you are recommending we remove this from WebKit. The comment should explain the circumstances where we would remove it. Simply saying deprecated is far too vague. It doesn’t say who deprecated it or why it wasn’t simply removed.

[Luiz Agostini]

Created attachment 71215 [details]
patch

[Luiz Agostini]

(In reply to comment #77)
> (From update of attachment 69310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69310&action=review
>
> Looks good. Still some work to be done I think.
>
> > WebCore/bindings/scripts/CodeGeneratorJS.pm:289
> > +    if ($type eq "MediaQueryListListener") {
> > +        $implIncludes{"MediaQueryListListener.h"} = 1;
> > +    }
>
> It’s really disappointing that this is necessary. Adding a new type to the bindings generator is a lot of extra work.
>
> Specifically, you need to add test cases to the bindings tests. Take a look at the run-bindings-tests script, the test cases in WebCore/bindings/scripts/test, and the expected test results in the subdirectories of that directory. You’ll need to add test cases, and update the expected test results.

I am a bit confused by this because I have not seen any tests taking care of types that need special handling, like SerializedScriptValue and IDBKey. To add tests here would add several (13) files to this patch, some of them quite big. Could I do that in another patch?

>
> > WebCore/css/MediaQueryMatcher.h:104
> > +    ~MediaQueryMatcher();
>
> Once you change m_listeners, you probably won’t need an explicit destructor declaration.
>
> > WebCore/css/MediaQueryMatcher.h:120
> > +        ~Listener();
>
> You don’t need this explicit destructor declaration.

>
> In the WebKit project we normally use // comments, not /* */ comments, except for the license comment.
>
> > WebCore/css/MediaQueryList.h:50
> > +    ~MediaQueryList();
>
> Why do we need to explicitly define the destructor? Did you try with the default constructor.

I decided to do this way to avoid needing to include files MediaQueryMatcher.h and MediaList.h in this header file.

>
> There are many who would applaud you for writing extensive documentation. I am concerned that this is a long explanation which may quickly get out of sync with the code. A lot of this is worded in a way that I find confusing, with hypothetical situations that I don’t completely understand.

I agree. At some point the explanation would be needed in the review process, I think, but I am happy to remove it from the code

>
> > WebCore/dom/Document.cpp:2855
> > +    if (m_mediaQueryMatcher)
> > +        m_mediaQueryMatcher->evaluate();
>
> This doesn’t seem right to me. I don’t understand why the operation is named evaluate, nor why styleSelectorChanged is the right time to do it. It seems to me we don’t want to do any new evaluation until someone calls matchMedia.

The name is really bad and I renamed it to styleSelectorChanged.
Every time the Document::styleSelectorChanged runs all the queries are evaluated and if the result of any of them has changed the corresponding listener is called.

>
> > WebCore/css/MediaQueryListListener.cpp:53
> > +bool MediaQueryListListener::isEqualTo(ScriptState* state, const MediaQueryListListener* other)
> > +{
> > +    return other && other->m_value.isEqual(state, m_value);
> > +}
>
> The use of JavaScript equality to define listener equality is extremely unusual; other listeners use object identity, not object equality, which is both more efficient and never involves running arbitrary JavaScript, while isEqualTo can involve running script. We need test cases covering this, and clarifying that it’s equality and not strict equality. I don’t see coverage in the tests you added.
>
> > WebCore/css/MediaQueryMatcher.cpp:138
> > +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> > +        if (m_listeners[i]->listener()->isEqualTo(exec, listener.get()) && m_listeners[i]->query() == query.get())
> > +            return;
> > +    }
>
> This code can reenter because a JavaScript equality comparison can involve running arbitrary script. We need to add some test cases covering that. If we do this wrong we could end up adding a security vulnerability. I doubt very much that the JavaScript equality rule is a good one.

What I need is to compare javascript functions provided by the script and not MediaQueryListener  objects, because a MediaQueryListener object is created every time matchMedia is called from the script.
If the same javascript function is passed to matchMedia twice, two MediaQueryListener objects will be created. What I want is to consider those objects equal, otherwise the same callback could be registered more then once for the same query.
I think that the same is done for event listeners.
Every time a listener is added or removed it is compared to all other listeners.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/MediaQueryListListener.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 39 files


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

[Darin Adler]

(In reply to comment #79)
> (In reply to comment #77)
> > Specifically, you need to add test cases to the bindings tests. Take a look at the run-bindings-tests script, the test cases in WebCore/bindings/scripts/test, and the expected test results in the subdirectories of that directory. You’ll need to add test cases, and update the expected test results.
> 
> I am a bit confused by this because I have not seen any tests taking care of types that need special handling, like SerializedScriptValue and IDBKey.

I did not review those patches. It is a bad idea to add special types without adding the tests.

> To add tests here would add several (13) files to this patch, some of them quite big. Could I do that in another patch?

Maybe. If your concern is patch size, then another way to do it is to add the test cases first, check in the test cases and the old “wrong” results, and then include only the changes to the test results in this patch.

> > There are many who would applaud you for writing extensive documentation. I am concerned that this is a long explanation which may quickly get out of sync with the code. A lot of this is worded in a way that I find confusing, with hypothetical situations that I don’t completely understand.
> 
> I agree. At some point the explanation would be needed in the review process, I think, but I am happy to remove it from the code

There are certainly some explanations that should be in comments. The trick is to find a way to explain things with comments small enough that people will read them.

> Every time the Document::styleSelectorChanged runs all the queries are evaluated and if the result of any of them has changed the corresponding listener is called.

That seems inefficient and seems like it will result in multiple calls to the listener when there could be only a single call.

> > > WebCore/css/MediaQueryListListener.cpp:53
> > > +bool MediaQueryListListener::isEqualTo(ScriptState* state, const MediaQueryListListener* other)
> > > +{
> > > +    return other && other->m_value.isEqual(state, m_value);
> > > +}
> >
> > The use of JavaScript equality to define listener equality is extremely unusual; other listeners use object identity, not object equality, which is both more efficient and never involves running arbitrary JavaScript, while isEqualTo can involve running script. We need test cases covering this, and clarifying that it’s equality and not strict equality. I don’t see coverage in the tests you added.
> >
> > > WebCore/css/MediaQueryMatcher.cpp:138
> > > +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> > > +        if (m_listeners[i]->listener()->isEqualTo(exec, listener.get()) && m_listeners[i]->query() == query.get())
> > > +            return;
> > > +    }
> >
> > This code can reenter because a JavaScript equality comparison can involve running arbitrary script. We need to add some test cases covering that. If we do this wrong we could end up adding a security vulnerability. I doubt very much that the JavaScript equality rule is a good one.
> 
> What I need is to compare javascript functions provided by the script and not MediaQueryListener objects, because a MediaQueryListener object is created every time matchMedia is called from the script.
> If the same javascript function is passed to matchMedia twice, two MediaQueryListener objects will be created. What I want is to consider those objects equal, otherwise the same callback could be registered more then once for the same query.
> I think that the same is done for event listeners.
> Every time a listener is added or removed it is compared to all other listeners.

When JavaScript event listeners are compared, the identity of the JavaScript function is checked, not the equality. See in JSEventListener::operator== that it compares m_jsFunction with the other m_jsFunction. This does not use JavaScript equality checking. It is wrong to use the JavaScript notion of equality here.

If the same JavaScript function is provided twice, then you will have the same object, not two objects that are equal.

It’s fine for MediaQueryListener to provide a comparison function that checks the equality of the script value contained inside it. But isEqual should not be involved.

[Darin Adler]

Comment on attachment 71215 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71215&action=review

> WebCore/css/MediaQueryListListener.cpp:52
> +    return other && other->m_value.isEqual(state, m_value);

The use of isEqual here is wrong. It should just be:

    return other && m_value == other->m_value;

And you need test cases covering this. Calling out to arbitrary JavaScript here as isEqual would can create major problems including security vulnerabilities.

[Luiz Agostini]

(In reply to comment #81)
> (In reply to comment #79)
> > (In reply to comment #77)
> > > Specifically, you need to add test cases to the bindings tests. Take a look at the run-bindings-tests script, the test cases in WebCore/bindings/scripts/test, and the expected test results in the subdirectories of that directory. You’ll need to add test cases, and update the expected test results.
> >
> > I am a bit confused by this because I have not seen any tests taking care of types that need special handling, like SerializedScriptValue and IDBKey.
>
> I did not review those patches. It is a bad idea to add special types without adding the tests.
>
> > To add tests here would add several (13) files to this patch, some of them quite big. Could I do that in another patch?
>
> Maybe. If your concern is patch size, then another way to do it is to add the test cases first, check in the test cases and the old “wrong” results, and then include only the changes to the test results in this patch.
>

Good!! This way the effects of the changes in generators will be clear. I created that patch. Please see bug 48028.

> > Every time the Document::styleSelectorChanged runs all the queries are evaluated and if the result of any of them has changed the corresponding listener is called.
>
> That seems inefficient and seems like it will result in multiple calls to the listener when there could be only a single call.

I took care to make sure that the pair (query, listener) will be registered only once and that each query will be evaluated only once even if it apears in more then one pair (query, listener).
This way I believe that a listener will be called more then once only if it is associated to more then one query. As the query is the parameter of the listener, each time a listener is called it receives a different parameter (its associated query).

>
> > > > WebCore/css/MediaQueryListListener.cpp:53
> > > > +bool MediaQueryListListener::isEqualTo(ScriptState* state, const MediaQueryListListener* other)
> > > > +{
> > > > +    return other && other->m_value.isEqual(state, m_value);
> > > > +}
> > >
> > > The use of JavaScript equality to define listener equality is extremely unusual; other listeners use object identity, not object equality, which is both more efficient and never involves running arbitrary JavaScript, while isEqualTo can involve running script. We need test cases covering this, and clarifying that it’s equality and not strict equality. I don’t see coverage in the tests you added.
> > >
> > > > WebCore/css/MediaQueryMatcher.cpp:138
> > > > +    for (size_t i = 0; i < m_listeners.size(); ++i) {
> > > > +        if (m_listeners[i]->listener()->isEqualTo(exec, listener.get()) && m_listeners[i]->query() == query.get())
> > > > +            return;
> > > > +    }
> > >
> > > This code can reenter because a JavaScript equality comparison can involve running arbitrary script. We need to add some test cases covering that. If we do this wrong we could end up adding a security vulnerability. I doubt very much that the JavaScript equality rule is a good one.
> >
> > What I need is to compare javascript functions provided by the script and not MediaQueryListener objects, because a MediaQueryListener object is created every time matchMedia is called from the script.
> > If the same javascript function is passed to matchMedia twice, two MediaQueryListener objects will be created. What I want is to consider those objects equal, otherwise the same callback could be registered more then once for the same query.
> > I think that the same is done for event listeners.
> > Every time a listener is added or removed it is compared to all other listeners.
>
> When JavaScript event listeners are compared, the identity of the JavaScript function is checked, not the equality. See in JSEventListener::operator== that it compares m_jsFunction with the other m_jsFunction. This does not use JavaScript equality checking. It is wrong to use the JavaScript notion of equality here.
>
> If the same JavaScript function is provided twice, then you will have the same object, not two objects that are equal.
>
> It’s fine for MediaQueryListener to provide a comparison function that checks the equality of the script value contained inside it. But isEqual should not be involved.

Now I see what you mean. I did not see it before because JS version of ScriptValue did not have an operator==. I added it and now I am using it for comparing MediaQueryListeners.
I also added a method isFunction to ScriptValue to avoid registering not callable javascript values as listeners.


The new patch now will depend on the other (bug 48028). I am working in new layout tests and will submit a new patch soon.

[Luiz Agostini]

Created attachment 71513 [details]
patch

[Luiz Agostini]

Created attachment 71932 [details]
patch

rebase

[Luiz Agostini]

Hey Darin, did I finally understand the problem with the callback comparisons? Would you mind taking a look at the last patch?

[Luiz Agostini]

Created attachment 73769 [details]
patch

Previous patch was getting old. Rebasing.

[Darin Adler]

Comment on attachment 73769 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73769&action=review

This looks great. I don’t understand the round trip from document to frame and then back to document, and I’m worried that it might even possibly create a security bug. So please fix at least that.

> WebCore/bindings/js/ScriptValue.cpp:91
> +    CallType callType = getCallData(m_value, callData);
> +    return callType != CallTypeNone;

No need for the callType local variable here.

> WebCore/bindings/js/ScriptValue.h:61
> +    bool isFunction() const;
> +    bool operator==(const ScriptValue& other) const { return m_value == other.m_value; }

A little strange to group these two functions together in a paragraph. isFunction seems to belong with isObject, while operator== perhaps should be off on its own.

> WebCore/bindings/scripts/CodeGeneratorGObject.pm:217
> +            $param->type eq "MediaQueryListListener") {

Always a bad sign when we have to special case a new type.

> WebCore/css/MediaQueryList.cpp:96
> +        setMatches(m_matcher->evaluate(m_media.get()));

I don’t think the factoring here is quite optimal. I’d name the helper function updateMatches instead of setMatches, and have it do the m_matcher->evaluate call as well.

> WebCore/css/MediaQueryList.h:29
> +#include <wtf/PassRefPtr.h>

No need to include the PassRefPtr header here. Instead you could include Forward.h.

> WebCore/css/MediaQueryList.h:32
> +#include <wtf/text/WTFString.h>

No need to include the WTFString header here. Instead you could include Forward.h.

> WebCore/css/MediaQueryList.h:44
> +//  MediaQueryList interface is specified at http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface
> +//  The objects of this class are returned by window.matchMedia. They may be used to
> +//  retrieve the current value of the given media query and to add/remove listeners that
> +//  will be called whenever the value of the query changes.

No need for the double spaces at the beginnings of lines here.

> WebCore/css/MediaQueryList.h:66
> +    int m_evaluationRound; // Indicates if the query has been evaluated after the last style selector change.
> +    int m_changeRound; // Used to know if the query has changed in the last style selector change.

I think unsigned would be better than int.

> WebCore/css/MediaQueryListListener.h:39
> +//  See http://dev.w3.org/csswg/cssom-view/#the-mediaquerylist-interface

No need for double spaces here.

> WebCore/css/MediaQueryListListener.h:50
> +    bool isEqualTo(const MediaQueryListListener*);

Why not use operator== for this instead of a named function?

> WebCore/css/MediaQueryMatcher.cpp:90
> +    Document* document = m_document->frame()->document();
> +    if (!document)
> +        return 0;

No need to null check the document. All frames have documents.

I don’t understand why you go from the document to the frame and then back to the document.

> WebCore/css/MediaQueryMatcher.cpp:126
> +    if (!m_document || !m_document->frame())
> +        return;

Why check frame against 0? Seems unnecessary.

> WebCore/css/MediaQueryMatcher.cpp:138
> +    if (!m_document || !m_document->frame())

Why check frame against 0? Seems unnecessary.

> WebCore/css/MediaQueryMatcher.h:34
> +#include "ScriptState.h"
> +
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/Vector.h>
> +#include <wtf/text/WTFString.h>

Includes go in a single paragraph, not two.

No need to include PassRefPtr.h and WTFString.h. Instead you should use Forward.h.

> WebCore/css/MediaQueryMatcher.h:47
> +//  MediaQueryMatcher class is responsible for keeping a vector of pairs
> +//  MediaQueryList x MediaQueryListListener. It is responsible for evaluating the queries
> +//  whenever it is needed and to call the listeners if the corresponding query has changed.
> +// The listeners must be called in the very same order in which they have been added.

No need for those extra spaces at the beginning of each comment line.

> WebCore/css/MediaQueryMatcher.h:49
> +class MediaQueryMatcher : public RefCounted<MediaQueryMatcher> {

This seems to be an object with a single owner, the document. And nothing else seems to use a RefPtr on it. Accordingly, I suggest making it be a single-owner object, not a reference counted one.

> WebCore/css/MediaQueryMatcher.h:60
> +    int evaluationRound() const { return m_evaluationRound; }

I think unsigned would be better than int. To be pedantic, unsigned has well defined overflowing semantics, whereas int does not.

> WebCore/css/MediaQueryMatcher.h:90
> +    int m_evaluationRound;

I think unsigned would be better than int.

[Luiz Agostini]

(In reply to comment #88)
> (From update of attachment 73769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73769&action=review

> 
> > WebCore/css/MediaQueryMatcher.h:49
> > +class MediaQueryMatcher : public RefCounted<MediaQueryMatcher> {
> 
> This seems to be an object with a single owner, the document. And nothing else seems to use a RefPtr on it. Accordingly, I suggest making it be a single-owner object, not a reference counted one.

Actually MediaQueryList uses a RefPtr on MediaQueryMatcher.

> 
> > WebCore/css/MediaQueryMatcher.h:60
> > +    int evaluationRound() const { return m_evaluationRound; }
> 
> I think unsigned would be better than int. To be pedantic, unsigned has well defined overflowing semantics, whereas int does not.

Comments and information are always welcome.

[Luiz Agostini]

Created attachment 74474 [details]
patch

[Darin Adler]

Comment on attachment 74474 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74474&action=review

> WebCore/ChangeLog:54
> +        publishes methods matchMedia but for page cache to work properly  the reference to the

Stray space here between in “ properly  the reference”.

> WebCore/css/MediaQueryList.cpp:96
> +        setMatches(m_matcher->evaluate(m_media.get()));

A comment I made last time: I don’t think the factoring here is quite optimal. I’d name the helper function updateMatches instead of setMatches, and have it do the m_matcher->evaluate call as well.

> WebCore/css/MediaQueryList.h:54
> +    void addListener(PassRefPtr<MediaQueryListListener>);
> +    void removeListener(PassRefPtr<MediaQueryListListener>);

I understand why the addListener function takes a PassRefPtr, but the removeListener function should not.

> WebCore/css/MediaQueryMatcher.cpp:125
> +        if (*m_listeners[i]->listener() == *listener.get() && m_listeners[i]->query() == query.get())

Do we need to say get() here? I think we could remove both calls to get() and the code would still compile and work fine.

> WebCore/css/MediaQueryMatcher.cpp:137
> +    for (int i = m_listeners.size() - 1; i >= 0; --i) {

Why iterate backwards? This unnecessarily truncates a size_t to an int, and since we always stop as soon as we find what we are looking for, there is no benefit to iterating in a certain direction.

> WebCore/css/MediaQueryMatcher.cpp:138
> +        if (*m_listeners[i]->listener() == *listener.get() && m_listeners[i]->query() == query.get()) {

Do we need to say get() here? I think we could remove both calls to get() and the code would still compile and work fine.

> WebCore/css/MediaQueryMatcher.cpp:149
> +    ScriptState* exec = mainWorldScriptState(m_document->frame());

I don’t think “exec” is good name. How about “state” or “scriptState”?

> WebCore/css/MediaQueryMatcher.h:54
> +    void addListener(PassRefPtr<MediaQueryListListener>, PassRefPtr<MediaQueryList>);
> +    void removeListener(PassRefPtr<MediaQueryListListener>, PassRefPtr<MediaQueryList>);

I understand why the addListener command takes PassRefPtr, but the removeListener command should not.

> WebCore/page/DOMWindow.idl:146
> +        // styleMedia has been removed from the cssom-view specification.
>          readonly attribute StyleMedia styleMedia;
> +        MediaQueryList matchMedia(in DOMString query);

That comment is only about styleMedia, so it should be paragraphed with styleMedia and not with matchMedia; you could put matchMedia first in the file and add a blank line.

Also, “cssom-view specification” doesn’t look quite right in all lower case.

[Luiz Agostini]

(In reply to comment #91)
> (From update of attachment 74474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74474&action=review

> 
> > WebCore/css/MediaQueryList.cpp:96
> > +        setMatches(m_matcher->evaluate(m_media.get()));
> 
> A comment I made last time: I don’t think the factoring here is quite optimal. I’d name the helper function updateMatches instead of setMatches, and have it do the m_matcher->evaluate call as well.
> 

Sorry, I did not comment on this last time. I did this way because setMatches is used in two situations and in each one the parameter comes from different methods. They are:

void MediaQueryList::evaluate(MediaQueryEvaluator* evaluator, bool& notificationNeeded)
{
    if (m_evaluationRound != m_matcher->evaluationRound() && evaluator)
        setMatches(evaluator->eval(m_media.get()));
    notificationNeeded = m_changeRound == m_matcher->evaluationRound();
}

and

bool MediaQueryList::matches()
{
    if (m_evaluationRound != m_matcher->evaluationRound())
        setMatches(m_matcher->evaluate(m_media.get()));
    return m_matches;
}

The reason to do this is to have a small optimization in MediaQueryMatcher::styleSelectorChanged when all the queries that have listeners are being evaluated and the same MediaQueryEvaluator instance is being used to evaluate all of them.


> > WebCore/css/MediaQueryList.h:54
> > +    void addListener(PassRefPtr<MediaQueryListListener>);
> > +    void removeListener(PassRefPtr<MediaQueryListListener>);
> 
> I understand why the addListener function takes a PassRefPtr, but the removeListener function should not.
> 

The point is that a PassRefPtr<MediaQueryListListener> is comming from JS generated code. I adjusted MediaQueryMatcher::removeListener to receive pointer instead of PassRefPtr<> but could not do the same for MediaQueryList::removeListener.


I will land this patch with all the changes that you requested in previous comment except for the ones I commented here.
But of course I will make any change you consider important in future bugs.

Thank you very much for your help and review!

[Luiz Agostini]

Committed r72552: <http://trac.webkit.org/changeset/72552>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72552 might have broken Qt Linux Release
The following tests are not passing:
fast/dom/Window/window-properties.html
fast/dom/Window/window-property-descriptors.html
fast/dom/prototype-inheritance.html

[Marja Hölttä]

FYI: this old patch is causing memory leaks, see bug 103242. It's atm unclear how to solve it properly.