Add error dispatch events to SVGStyleElement

Add error dispatch events to SVGStyleElement

This is a follow up to https://codereview.chromium.org/1032033002,
adding the plumbing to SVGStyleElements for dispatching error events. In
turn, this provides the support for firing error events on CSP hash and
nonce failures.

R=pdr@chromium.org

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

[jww, 2015-03-31 00:12:07]

pdr@, this is the addition we discussed to add error events to SVG style
elements.

[pdr., 2015-03-31 03:01:43]

Looking good! Just minor comments below.

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
File Source/core/html/HTMLStyleElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
Source/core/html/HTMLStyleElement.cpp:146: if (m_firedLoad && errorStatus ==
NoErrorLoadingSubresource)
This looks like it can lead to firing more than one load event.

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:106: bool sendSVGErrorEventIfPossible();
Should this be void instead of bool?

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:109: virtual Timer<SVGElement>*
svgErrorEventTimer();
Can you put the above 4 functions (err, it will be 3) on SVGStyleElement?

I thought they might be here for sharing with SVGScriptElement, but it looks
like that would go through shared code with ScriptLoader.

[fs, 2015-03-31 09:03:38]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-03-31 09:03:38]

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:852: if (!haveLoadedRequiredResources())
Why would we want to gate this on this (old oddity)?

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:109: virtual Timer<SVGElement>*
svgErrorEventTimer();
On 2015/03/31 03:01:43, pdr wrote:
> Can you put the above 4 functions (err, it will be 3) on SVGStyleElement?

Agreed - with the below twist.

> I thought they might be here for sharing with SVGScriptElement, but it looks
> like that would go through shared code with ScriptLoader.

Could we try using EventSender and Co. instead to converge (rather than diverge)
with HTMLStyleElement? The 'load' event crazies may have shaped the current
solution, but I don't think we should be basing "new" stuff on that structure.

[jww, 2015-03-31 21:31:42]

I believe this addresses pdr@ and fs@'s comments. Let me know!

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
File Source/core/html/HTMLStyleElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
Source/core/html/HTMLStyleElement.cpp:146: if (m_firedLoad && errorStatus ==
NoErrorLoadingSubresource)
On 2015/03/31 at 03:01:43, pdr wrote:
> This looks like it can lead to firing more than one load event.

I'm fairly sure this is correct, but double check my reasoning :-) If it's a
load event, then errorStatus will be NoErrorLoadingSubresource, in which case if
one has already fired, m_firedLoad will be true, and this condition will hold,
this returning early. Otherwise, if the event being fired is *not* a load event
(namely if it's an error), the condition will fail, and it will not return
early.

A convoluted way of saying that the only way to not return early is that a load
has not fired yet *and* the event *isn't* a load.

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:852: if (!haveLoadedRequiredResources())
On 2015/03/31 at 09:03:38, fs wrote:
> Why would we want to gate this on this (old oddity)?

Hm, nope, I think not. That's a remnant of the copy from the load event code.

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:106: bool sendSVGErrorEventIfPossible();
On 2015/03/31 at 03:01:43, pdr wrote:
> Should this be void instead of bool?

Yes, although this goes away given fs@'s comments below.

https://codereview.chromium.org/1049513003/diff/20001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:109: virtual Timer<SVGElement>*
svgErrorEventTimer();
On 2015/03/31 at 09:03:38, fs wrote:
> On 2015/03/31 03:01:43, pdr wrote:
> > Can you put the above 4 functions (err, it will be 3) on SVGStyleElement?
> 
> Agreed - with the below twist.
> 
> > I thought they might be here for sharing with SVGScriptElement, but it looks
> > like that would go through shared code with ScriptLoader.
> 
> Could we try using EventSender and Co. instead to converge (rather than
diverge) with HTMLStyleElement? The 'load' event crazies may have shaped the
current solution, but I don't think we should be basing "new" stuff on that
structure.

Done.

[pdr., 2015-03-31 21:36:38]

LGTM.

Could you please give fs@opera.com a day before committing?

https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
File Source/core/svg/SVGStyleElement.h (right):

https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.h:28: #include "platform/Timer.h"
Nit: is this include needed?

[jww, 2015-03-31 22:39:58]

On 2015/03/31 at 21:36:38, pdr wrote:
> LGTM.
> 
> Could you please give fs@opera.com a day before committing?
Will do.
> 
>
https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
> File Source/core/svg/SVGStyleElement.h (right):
> 
>
https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
> Source/core/svg/SVGStyleElement.h:28: #include "platform/Timer.h"
> Nit: is this include needed?
Removed.

[fs, 2015-04-01 08:47:51]

LGTM w/ nits and comments

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
File Source/core/html/HTMLStyleElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/20001/Source/core/html/HTMLSt...
Source/core/html/HTMLStyleElement.cpp:146: if (m_firedLoad && errorStatus ==
NoErrorLoadingSubresource)
On 2015/03/31 21:31:42, jww wrote:
> On 2015/03/31 at 03:01:43, pdr wrote:
> > This looks like it can lead to firing more than one load event.
> 
> I'm fairly sure this is correct, but double check my reasoning :-) If it's a
> load event, then errorStatus will be NoErrorLoadingSubresource, in which case
if
> one has already fired, m_firedLoad will be true, and this condition will hold,
> this returning early. Otherwise, if the event being fired is *not* a load
event
> (namely if it's an error), the condition will fail, and it will not return
> early.
> 
> A convoluted way of saying that the only way to not return early is that a
load
> has not fired yet *and* the event *isn't* a load.

Maybe something like:

bool sheetWasLoaded = errorStatus == NoErrorLoadingSubresource;
if (m_firedLoad && sheetWasLoaded)
    return;
m_loadedSheet = ...;

would make it feel less convoluted.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:786: static bool hasEventListener(Element*
element, const AtomicString& eventType)
Nit: This change is no longer needed.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
File Source/core/svg/SVGStyleElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.cpp:29: #include "core/events/EventSender.h"
Nit: Already included in the header.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.cpp:52: }
Want

styleErrorEventSender().cancelEvent(this);

here I think.

[jww, 2015-04-01 16:26:58]

https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
File Source/core/svg/SVGStyleElement.h (right):

https://codereview.chromium.org/1049513003/diff/40001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.h:28: #include "platform/Timer.h"
On 2015/03/31 at 21:36:38, pdr wrote:
> Nit: is this include needed?

Removed.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:786: static bool hasEventListener(Element*
element, const AtomicString& eventType)
On 2015/04/01 at 08:47:51, fs wrote:
> Nit: This change is no longer needed.

Done.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
File Source/core/svg/SVGStyleElement.cpp (right):

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.cpp:29: #include "core/events/EventSender.h"
On 2015/04/01 at 08:47:51, fs wrote:
> Nit: Already included in the header.

Done.

https://codereview.chromium.org/1049513003/diff/60001/Source/core/svg/SVGStyl...
Source/core/svg/SVGStyleElement.cpp:52: }
On 2015/04/01 08:47:51, fs wrote:
> Want
> 
> styleErrorEventSender().cancelEvent(this);
> 
> here I think.

Done.

[jww, 2015-04-01 16:27:04]

The CQ bit was checked by jww@chromium.org

[jww, 2015-04-01 16:27:04]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
fs@opera.com
Link to the patchset: https://codereview.chromium.org/1049513003/#ps80001
(title: "Nits")

[commit-bot: I haz the power, 2015-04-01 16:27:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1049513003/80001

[commit-bot: I haz the power, 2015-04-01 19:14:14]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192956