[svg] Make svgView override existing data selectively

[svg] Make svgView override existing data selectively.

The change is to not blow off all the existing viewBox,
pAR, etc when an #svgView(...) is used. Instead only
replace the parts that are actually specified in the
svgView syntax.

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

[Erik Dahlström (inactive), 2015-03-18 10:33:20]

ed@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[fs, 2015-03-18 13:43:09]

https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGSVGEleme...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGSVGEleme...
Source/core/svg/SVGSVGElement.cpp:716: view->reset();
It feels like this ought to go together with the currentView()-call above - if
we had a viewspec already, then it would've been reset at the beginning of this
function.

I wonder if this means that this should be done already on creation though?

https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGViewSpec...
File Source/core/svg/SVGViewSpec.cpp (right):

https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGViewSpec...
Source/core/svg/SVGViewSpec.cpp:74: if (m_contextElement) {
This is pretty similar to inheritViewAttributes in SVGSVGElement. Do you think
it would work to something like:

<old reset(): just set defaults>
if (m_contextElement)
    inheritViewAttributesFromElement(m_contextElement);

where inheritViewAttributesFromElement was essentially this branch (but without
all the else-branches). inheritViewAttributes could then be implemented as:

view->...(this->bla());
...
view->inheritViewAttributesFromElement(viewElement);

It means some overwriting would take place though, but I suspect that nothing to
worry about. (Alternative would be to collect attribute in a defined order, and
then set.)

[Erik Dahlström (inactive), 2015-03-19 09:59:24]

On 2015/03/18 13:43:09, fs wrote:
>
https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGSVGEleme...
> File Source/core/svg/SVGSVGElement.cpp (right):
> 
>
https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGSVGEleme...
> Source/core/svg/SVGSVGElement.cpp:716: view->reset();
> It feels like this ought to go together with the currentView()-call above - if
> we had a viewspec already, then it would've been reset at the beginning of
this
> function.
> 
> I wonder if this means that this should be done already on creation though?
> 
>
https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGViewSpec...
> File Source/core/svg/SVGViewSpec.cpp (right):
> 
>
https://codereview.chromium.org/1015743003/diff/1/Source/core/svg/SVGViewSpec...
> Source/core/svg/SVGViewSpec.cpp:74: if (m_contextElement) {
> This is pretty similar to inheritViewAttributes in SVGSVGElement. Do you think
> it would work to something like:
> 
> <old reset(): just set defaults>
> if (m_contextElement)
>     inheritViewAttributesFromElement(m_contextElement);
> 
> where inheritViewAttributesFromElement was essentially this branch (but
without
> all the else-branches). inheritViewAttributes could then be implemented as:
> 
> view->...(this->bla());
> ...
> view->inheritViewAttributesFromElement(viewElement);
> 
> It means some overwriting would take place though, but I suspect that nothing
to
> worry about. (Alternative would be to collect attribute in a defined order,
and
> then set.)

Ok, reworked this in patchset #2 so that it's more explicit about how things are
inherited. PTAL.

[fs, 2015-03-19 10:16:30]

LGTM w/ 'view'-related concern

https://codereview.chromium.org/1015743003/diff/20001/Source/core/svg/SVGSVGE...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1015743003/diff/20001/Source/core/svg/SVGSVGE...
Source/core/svg/SVGSVGElement.cpp:738: m_useCurrentView = true;
Maybe we should tread more carefully here, since viewElement.ownerSVGElement()
is not actually guaranteed to return 'this'.

[Erik Dahlström (inactive), 2015-03-19 13:39:07]

The CQ bit was checked by ed@opera.com

[Erik Dahlström (inactive), 2015-03-19 13:39:07]

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

[commit-bot: I haz the power, 2015-03-19 13:39:20]

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

[commit-bot: I haz the power, 2015-03-19 15:13:31]

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