Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
300223009:
Implement basic parts of hit regions on canvas2d. (Closed)
|
Created:
6 years, 6 months ago by zino Modified:
6 years, 6 months ago CC:
blink-reviews, arv+blink, blink-reviews-html_chromium.org, abarth-chromium, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, aandrey+blink_chromium.org, watchdog-blink-watchlist_google.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement basic parts of hit regions on canvas2d.
Specification:
http://www.w3.org/TR/2dcontext/#hit-regions
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tRFguWa-oZc
BUG=328961
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176674
Patch Set 1 : #Patch Set 2 : rebase #
Total comments: 64
Patch Set 3 : oilpan #Patch Set 4 : remove comment and exception message #Patch Set 5 : take clipRegion into account #
Total comments: 4
Patch Set 6 : addressed some comments #Patch Set 7 : AX and test #Patch Set 8 : add clip tests #
Total comments: 16
Patch Set 9 : addressed comments #
Total comments: 2
Patch Set 10 : add isClipMode() and check bounding box #
Total comments: 2
Patch Set 11 : lowercase 'f' #
Total comments: 4
Patch Set 12 : addressed some nits #Patch Set 13 : rebase #Patch Set 14 : update test expectations #Patch Set 15 : add header #
Total comments: 3
Patch Set 16 : exclude clipping region part #
Total comments: 2
Messages
Total messages: 78 (0 generated)
Dear all, I implemented basic parts of hit regions on canvas2d. Please take a look. Thank you :)
I see a lot of oilpan related problems. Please build locally with oilpan enabled to make sure your code is okay. https://codereview.chromium.org/300223009/diff/140001/Source/bindings/v8/Dict... File Source/bindings/v8/Dictionary.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/bindings/v8/Dict... Source/bindings/v8/Dictionary.h:182: bool getWithUndefinedOrNullCheck(const String&, RefPtr<Element>&) const; Should be RefPtrWillBeMember, and the corresponding members of HitRegionOptions should also use RefPtrWillBeMember. And don't forget the trace method on HitRegionOptions. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.h:357: OwnPtrWillBeMember<HitRegionManager> m_hitRegionManager; If this will be a member, you need to update the trace function. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:20: struct HitRegionOptions { GarbageCollected? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:22: RefPtrWillBeMember<Element> control; missing trace method https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:26: class HitRegion FINAL : public RefCountedWillBeGarbageCollectedFinalized<HitRegion> { I don't think this class needs to be finalized. It does not even have a destructor. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:30: return adoptRef(new HitRegion(options)); adoptRef -> adoptRefWillBeNoop https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:46: RefPtrWillBeMember<Element> m_control; missing trace method https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:50: class HitRegionManager FINAL : public NoBaseWillBeGarbageCollectedFinalized<HitRegionManager> { Does this class really need to be finalized? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:55: virtual ~HitRegionManager() { } Is this necessary? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:79: HitRegionList m_hitRegionList; This member and the one below need to be traced.
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:88: , m_hitRegionManager(HitRegionManager::create()) do you want to do this lazily? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2374: passOptions.control = nullptr; why are you doing this? It's ok for a control to start out outside the canvas and then moved to the canvas later. Better not to check for isdescendant here. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.idl:143: [RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute unsigned short hitRegionsCount; will this be removed later then?
I'm happy to take a closer look once this CL passes enable_oilpan=1 build. When you compile with enable_oilpan=1, a clang plugin will detect syntax errors related to oilpan. I'm happy to help you debug, so feel free to ping me if you have any trouble :) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:20: struct HitRegionOptions { On 2014/06/02 22:56:13, junov wrote: > GarbageCollected? I think this is always stack-allocated. So you can add: STACK_ALLOCATED(); and use: RefPtrWillBeMember<Element> control; You don't need trace it since oilpan's conservative scanning will find on-stack pointers. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:26: class HitRegion FINAL : public RefCountedWillBeGarbageCollectedFinalized<HitRegion> { On 2014/06/02 22:56:13, junov wrote: > I don't think this class needs to be finalized. It does not even have a > destructor. Unfortunately this needs a destructor, since a String (m_id) requires a finalization. The GC plugin will catch this kind of error when compiling with enable_oilpan=1. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:43: HitRegion(const HitRegionOptions&); Add explicit. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:77: typedef WillBeHeapHashMap< const Element*, RefPtrWillBeMember<HitRegion> > HitRegionControlMap; How is it guaranteed that the Element raw pointer doesn't become stale? Also I guess you can use WillBeHeapHashMap<Member<const Element>, RefPtrWillBeMember<HitRegion>> since this map can have a strong reference to the Elements.
https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html (right): https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html:6: <body style="padding : 0; margin : 0;"> Probably would be good to have these be != 0 - either in this test, or another test. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2371: bool controlIsNull = !options.getWithUndefinedOrNullCheck("control", passOptions.control) || !passOptions.control; controlIsNull seems redundant with passOptions.control. (Ditto for idIsNull vs. passOptions.id.isEmpty()) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); Suggest you drop " or undefined or invalid type". (If 'id' is specified and undefined, the default value - null - will be used. Similarly I think an incorrect type should've been handled by the ES -> IDL dictionary conversion [which isn't implemented yet in Blink.) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2383: if (m_path.isEmpty()) { IIRC, Path::isEmpty means "does not have any segments". The W3 ED seems to say "If the specified path has no pixels, ...", which suppose could be interpreted this way, but could also be interpreted as "does not intersect any (CSS) pixel centers". Rik? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.idl:143: [RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute unsigned short hitRegionsCount; On 2014/06/02 22:57:40, Rik wrote: > will this be removed later then? Move to Internals? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:35: obj->setElementRect(elementRect); This might be better answered by dmazzoni, but this looks odd to me (a later caller could end up overwriting the rect in cases where two controls share an ancestor.) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:44: // FIXME: We should avoid following boundingRect test if fillRule is evenodd. Why? AFAICT this broad-phase check is equally good for both fill-rules https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:74: typedef WillBeHeapListHashSet< RefPtrWillBeMember<HitRegion> >::const_reverse_iterator HitRegionIterator; If you move this after the HitRegionList typedef it'll be simpler (and more obvious IMHO.) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:81: HitRegionControlMap m_hitRegionControlMap; I didn't see this being used for anything (just accounted). Drop until it's actually used?
https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html (right): https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html:16: window.axButton1 = window.accessibilityController.accessibleElementById("button1"); You don't need "window." anywhere in this test - local variables should be fine. Some tests set globals (window.axButton) when the check (shouldBe) is in a different context, like in a timeout callback - but that isn't needed here. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2370: bool idIsNull = !(options.getWithUndefinedOrNullCheck("id", passOptions.id)) || passOptions.id.isEmpty(); "null" isn't quite accurate since it could be empty or undefined. How about reversing it to "hasId" and "hasControl"? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/03 11:47:01, fs wrote: > Suggest you drop " or undefined or invalid type". (If 'id' is specified and > undefined, the default value - null - will be used. Similarly I think an > incorrect type should've been handled by the ES -> IDL dictionary conversion > [which isn't implemented yet in Blink.) What about an empty string? https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:26: if (AXObject* obj = axObjectCache->getOrCreate(m_control.get())) { Let's move some of this logic to a new function on AXObjectCache. Something like: axObjectCache->setCanvasObjectBounds(m_control.get(), elementRect) then have axObjectCache do the logic of walking up the ancestors In general we want directories outside of core/accessibility to only interact with AXObjectCache and not with AXObject. Long-term we hope to modularize it this way. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:35: obj->setElementRect(elementRect); On 2014/06/03 11:47:01, fs wrote: > This might be better answered by dmazzoni, but this looks odd to me (a later > caller could end up overwriting the rect in cases where two controls share an > ancestor.) Ideally it seems like we'd want container element bounding boxes to be the union of their child element bounding boxes. Perhaps it'd make sense to compute them on-demand rather than here?
https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html (right): https://codereview.chromium.org/300223009/diff/140001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html:16: window.axButton1 = window.accessibilityController.accessibleElementById("button1"); On 2014/06/03 17:26:10, dmazzoni wrote: > You don't need "window." anywhere in this test - local variables should be fine. > > Some tests set globals (window.axButton) when the check (shouldBe) is in a > different context, like in a timeout callback - but that isn't needed here. "local" variables in this scope will be on the global object (window) though. But the (explicit) qualification shouldn't be needed. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/03 17:26:10, dmazzoni wrote: > On 2014/06/03 11:47:01, fs wrote: > > Suggest you drop " or undefined or invalid type". (If 'id' is specified and > > undefined, the default value - null - will be used. Similarly I think an > > incorrect type should've been handled by the ES -> IDL dictionary conversion > > [which isn't implemented yet in Blink.) > > What about an empty string? Empty strings are coerced to null explicitly (addHitRegion() step 5, "If the arguments object's id member is an empty string, let it be null instead.") https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:35: obj->setElementRect(elementRect); On 2014/06/03 17:26:10, dmazzoni wrote: > On 2014/06/03 11:47:01, fs wrote: > > This might be better answered by dmazzoni, but this looks odd to me (a later > > caller could end up overwriting the rect in cases where two controls share an > > ancestor.) > > Ideally it seems like we'd want container element bounding boxes to be the union > of their child element bounding boxes. Perhaps it'd make sense to compute them > on-demand rather than here? Computing on-demand SGTM. https://codereview.chromium.org/300223009/diff/200001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/200001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2389: clipPath.addRect(clipBounds); clipBounds vs. CRC2D.clip() (i.e. what if it is a circle - or any other non-rectangular geometry really - add test?) https://codereview.chromium.org/300223009/diff/200001/Source/core/testing/Int... File Source/core/testing/Internals.idl (right): https://codereview.chromium.org/300223009/diff/200001/Source/core/testing/Int... Source/core/testing/Internals.idl:285: unsigned short countHitRegions(CanvasRenderingContext2D context); long seems more appropriate than unsigned short (since you return -1 for null context - mark context nullable?).
On 2014/06/03 11:47:00, fs wrote: > https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2383: if (m_path.isEmpty()) > { > IIRC, Path::isEmpty means "does not have any segments". The W3 ED seems to say > "If the specified path has no pixels, ...", which suppose could be interpreted > this way, but could also be interpreted as "does not intersect any (CSS) pixel > centers". Rik? This means that the width and height of the path's bounding box are non-zero and that the bounding box of the path falls within the canvas region.
I addressed some comments. But I should work about following items. - update accessibility (@dmazzoni, help me please T_T) - add some test cases - etc (some comments) Thank you. https://codereview.chromium.org/300223009/diff/140001/Source/bindings/v8/Dict... File Source/bindings/v8/Dictionary.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/bindings/v8/Dict... Source/bindings/v8/Dictionary.h:182: bool getWithUndefinedOrNullCheck(const String&, RefPtr<Element>&) const; On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > Should be RefPtrWillBeMember, and the corresponding members of HitRegionOptions > should also use RefPtrWillBeMember. And don't forget the trace method on > HitRegionOptions. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:88: , m_hitRegionManager(HitRegionManager::create()) On 2014/06/02 22:57:40, Rik wrote: > do you want to do this lazily? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2370: bool idIsNull = !(options.getWithUndefinedOrNullCheck("id", passOptions.id)) || passOptions.id.isEmpty(); On 2014/06/03 17:26:10, dmazzoni wrote: > "null" isn't quite accurate since it could be empty or undefined. How about > reversing it to "hasId" and "hasControl"? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2374: passOptions.control = nullptr; On 2014/06/02 22:57:40, Rik wrote: > why are you doing this? > It's ok for a control to start out outside the canvas and then moved to the > canvas later. > Better not to check for isdescendant here. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/04 08:13:40, fs wrote: > On 2014/06/03 17:26:10, dmazzoni wrote: > > On 2014/06/03 11:47:01, fs wrote: > > > Suggest you drop " or undefined or invalid type". (If 'id' is specified and > > > undefined, the default value - null - will be used. Similarly I think an > > > incorrect type should've been handled by the ES -> IDL dictionary conversion > > > [which isn't implemented yet in Blink.) > > > > What about an empty string? > > Empty strings are coerced to null explicitly (addHitRegion() step 5, "If the > arguments object's id member is an empty string, let it be null instead.") You're right but dictionary.get() can't do check null or undefined. So, following codes work abnormally. context.addHitRegion({ id : null // expected - empty string, actual - ??? }); context.addHitRegion({ id : undefined // expected - empty string, actual - 'undefined' }); There was a similar case. please refer to 'https://codereview.chromium.org/290143007' https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2383: if (m_path.isEmpty()) { On 2014/06/03 11:47:01, fs wrote: > IIRC, Path::isEmpty means "does not have any segments". The W3 ED seems to say > "If the specified path has no pixels, ...", which suppose could be interpreted > this way, but could also be interpreted as "does not intersect any (CSS) pixel > centers". Rik? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.h:357: OwnPtrWillBeMember<HitRegionManager> m_hitRegionManager; On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > If this will be a member, you need to update the trace function. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.idl:143: [RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute unsigned short hitRegionsCount; On 2014/06/03 11:47:01, fs wrote: > On 2014/06/02 22:57:40, Rik wrote: > > will this be removed later then? > > Move to Internals? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:44: // FIXME: We should avoid following boundingRect test if fillRule is evenodd. On 2014/06/03 11:47:01, fs wrote: > Why? AFAICT this broad-phase check is equally good for both fill-rules You're right. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:20: struct HitRegionOptions { On 2014/06/03 02:15:51, haraken wrote: > On 2014/06/02 22:56:13, junov wrote: > > GarbageCollected? > > I think this is always stack-allocated. So you can add: > > STACK_ALLOCATED(); > > and use: > > RefPtrWillBeMember<Element> control; > > You don't need trace it since oilpan's conservative scanning will find on-stack > pointers. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:20: struct HitRegionOptions { On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > GarbageCollected? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:22: RefPtrWillBeMember<Element> control; On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > missing trace method Please refer to @haraken's comment. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:26: class HitRegion FINAL : public RefCountedWillBeGarbageCollectedFinalized<HitRegion> { On 2014/06/03 02:15:51, haraken wrote: > On 2014/06/02 22:56:13, junov wrote: > > I don't think this class needs to be finalized. It does not even have a > > destructor. > > Unfortunately this needs a destructor, since a String (m_id) requires a > finalization. The GC plugin will catch this kind of error when compiling with > enable_oilpan=1. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:30: return adoptRef(new HitRegion(options)); On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > adoptRef -> adoptRefWillBeNoop Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:43: HitRegion(const HitRegionOptions&); On 2014/06/03 02:15:51, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:46: RefPtrWillBeMember<Element> m_control; On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > missing trace method Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:50: class HitRegionManager FINAL : public NoBaseWillBeGarbageCollectedFinalized<HitRegionManager> { On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > Does this class really need to be finalized? Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:55: virtual ~HitRegionManager() { } On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > Is this necessary? I'm not sure. if it wasn't exist, build error happened. (enable_oilpan=0) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:74: typedef WillBeHeapListHashSet< RefPtrWillBeMember<HitRegion> >::const_reverse_iterator HitRegionIterator; On 2014/06/03 11:47:01, fs wrote: > If you move this after the HitRegionList typedef it'll be simpler (and more > obvious IMHO.) Done. Good catch! Thank you :) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:77: typedef WillBeHeapHashMap< const Element*, RefPtrWillBeMember<HitRegion> > HitRegionControlMap; On 2014/06/03 02:15:51, haraken wrote: > > How is it guaranteed that the Element raw pointer doesn't become stale? > > Also I guess you can use WillBeHeapHashMap<Member<const Element>, > RefPtrWillBeMember<HitRegion>> since this map can have a strong reference to the > Elements. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:79: HitRegionList m_hitRegionList; On 2014/06/02 22:56:13, junov(OOO_back_june_16) wrote: > This member and the one below need to be traced. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:81: HitRegionControlMap m_hitRegionControlMap; On 2014/06/03 11:47:01, fs wrote: > I didn't see this being used for anything (just accounted). Drop until it's > actually used? It is actually used in addHitRegions(). Please refer to following the spec: - http://www.w3.org/TR/2dcontext/#dom-context-2d-addhitregion - 11. If there is a previous region with this control, remove it from the canvas element's hit region list. https://codereview.chromium.org/300223009/diff/200001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/200001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2389: clipPath.addRect(clipBounds); On 2014/06/04 08:13:40, fs wrote: > clipBounds vs. CRC2D.clip() > > (i.e. what if it is a circle - or any other non-rectangular geometry really - > add test?) Done. But I didn't add some tests yet. I'll upload them soon. https://codereview.chromium.org/300223009/diff/200001/Source/core/testing/Int... File Source/core/testing/Internals.idl (right): https://codereview.chromium.org/300223009/diff/200001/Source/core/testing/Int... Source/core/testing/Internals.idl:285: unsigned short countHitRegions(CanvasRenderingContext2D context); On 2014/06/04 08:13:40, fs wrote: > long seems more appropriate than unsigned short (since you return -1 for null > context - mark context nullable?). Done.
Dear all, I've just addressed all your comments. Could you please review again? Thank you :) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:26: if (AXObject* obj = axObjectCache->getOrCreate(m_control.get())) { On 2014/06/03 17:26:10, dmazzoni wrote: > Let's move some of this logic to a new function on AXObjectCache. > > Something like: axObjectCache->setCanvasObjectBounds(m_control.get(), > elementRect) > then have axObjectCache do the logic of walking up the ancestors > > In general we want directories outside of core/accessibility to only interact > with AXObjectCache and not with AXObject. Long-term we hope to modularize it > this way. Done. https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:35: obj->setElementRect(elementRect); On 2014/06/03 17:26:10, dmazzoni wrote: > On 2014/06/03 11:47:01, fs wrote: > > This might be better answered by dmazzoni, but this looks odd to me (a later > > caller could end up overwriting the rect in cases where two controls share an > > ancestor.) > > Ideally it seems like we'd want container element bounding boxes to be the union > of their child element bounding boxes. Perhaps it'd make sense to compute them > on-demand rather than here? Done.
Accessibility is looking pretty good, just a few comments. https://codereview.chromium.org/300223009/diff/300001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html (right): https://codereview.chromium.org/300223009/diff/300001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html:73: debug("Two depth containers tests."); nit: I'd phrase this as "Depth-two container tests" https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... File Source/core/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1250: if (node()->parentElement()->isInCanvasSubtree()) { OK for now, but add a FIXME indicating that this is inefficient if there are a lot of elements in the canvas. AXComputedObjectAttributeCache might be a good candidate for speeding this up. Again, don't worry about fixing in this change but please make a note of it. https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1264: return rect; If the resulting rect is still empty, fall through to the code below - a rect somewhere in the canvas is much better than an empty rect. https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1267: // AXNodeObjects have no mechanism yet to return a size or position. Delete this first bit of the comment and change it to say, if this object doesn't have an explicit element rect or one computable from its children, return the position of the ancestor that does have... https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:27: // Offset by the canvas rect (We should take border and padding into account). What do you mean by "we should take border and padding into account"? 1. Offset by the canvas rect, taking border and padding into account 2. Offset by the canvas rect. FIXME: we should take border and padding into account https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:36: if (!axObjectCache) Move this test up to the top; there's no reason to do the rest of the calculation if there's no AXObjectCache.
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/06 06:35:15, zino wrote: > On 2014/06/04 08:13:40, fs wrote: > > On 2014/06/03 17:26:10, dmazzoni wrote: > > > On 2014/06/03 11:47:01, fs wrote: > > > > Suggest you drop " or undefined or invalid type". (If 'id' is specified > and > > > > undefined, the default value - null - will be used. Similarly I think an > > > > incorrect type should've been handled by the ES -> IDL dictionary > conversion > > > > [which isn't implemented yet in Blink.) > > > > > > What about an empty string? > > > > Empty strings are coerced to null explicitly (addHitRegion() step 5, "If the > > arguments object's id member is an empty string, let it be null instead.") > > You're right but dictionary.get() can't do check null or undefined. You're not using "plain" get() though, but rather getWithUndefinedOrNullCheck() - which explicitly filters out null/undefined (i.e. giving nullable treatment). > So, following codes work abnormally. > > context.addHitRegion({ > id : null // expected - empty string, actual - ??? > }); > > context.addHitRegion({ > id : undefined // expected - empty string, actual - 'undefined' > }); In both of these cases getWithUndefinedOrNullCheck() will return false (and don't modify its out-parameter), so semantics-wise you're fine as long as you pass an isEmpty() String(). I.e. there'll be no ToString operation on null/undefined - which there would've been with "plain" get() (and with "plain" get you'd have 'null' [the string] in the first case.) > There was a similar case. please refer to > 'https://codereview.chromium.org/290143007' Since that is using lower-level primitives it's not directly comparable. (Tangent: It's also incorrect wrt how it treats null values, since only 'undefined' will cause default-value handling [WebIDL 4.2.20 step 5.1.4]) https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:81: HitRegionControlMap m_hitRegionControlMap; On 2014/06/06 06:35:16, zino wrote: > On 2014/06/03 11:47:01, fs wrote: > > I didn't see this being used for anything (just accounted). Drop until it's > > actually used? > > It is actually used in addHitRegions(). > > Please refer to following the spec: > - http://www.w3.org/TR/2dcontext/#dom-context-2d-addhitregion > - 11. If there is a previous region with this control, remove it from the > canvas element's hit region list. Ok. https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.cpp:1390: SkPath totalClipPath(path.skPath()); This doesn't appear to work in the general case - i.e. consider for instance a sequence of an empty element followed by a non-empty replace-op element (or really just a replace-op element). That may or may not be problematic in practice, but it'd probably make sense to at least document (and possibly assert) what isn't expected to work. https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.cpp:1401: totalClipPath = clipPath; Since the result of this method should be the intersection between |path| and the clip-stack, this won't do the right thing. Maybe it'd be better to simply compute the actual clip-path, and then perform the intersection against |path|?
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/09 11:06:26, fs wrote: > In both of these cases getWithUndefinedOrNullCheck() will return false (and > don't modify its out-parameter), so semantics-wise you're fine as long as you > pass an isEmpty() String(). I.e. there'll be no ToString operation on > null/undefined - which there would've been with "plain" get() (and with "plain" > get you'd have 'null' [the string] in the first case.) Thank you for detailed explanations :) BTW, I was wondering whether you'd like this to be fixed or not.
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/09 20:00:08, zino wrote: > On 2014/06/09 11:06:26, fs wrote: > > In both of these cases getWithUndefinedOrNullCheck() will return false (and > > don't modify its out-parameter), so semantics-wise you're fine as long as you > > pass an isEmpty() String(). I.e. there'll be no ToString operation on > > null/undefined - which there would've been with "plain" get() (and with > "plain" > > get you'd have 'null' [the string] in the first case.) > > Thank you for detailed explanations :) > BTW, I was wondering whether you'd like this to be fixed or not. I think fixing it would reduce clutter - and in the case of the exception message align more to where we want to be. So I suppose the exception message could stay for now if there's a test for it (which I believe there is.)
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/10 07:25:47, fs wrote: > I think fixing it would reduce clutter - and in the case of the exception > message align more to where we want to be. So I suppose the exception message > could stay for now if there's a test for it (which I believe there is.) You seems to want the exception message to be modified. Is my understanding correct? If so, I've already fixed since patch set 4. I was wondering about below your comment. ---------------------------------------------------------------------------------------------- controlIsNull seems redundant with passOptions.control. (Ditto for idIsNull vs. passOptions.id.isEmpty()) ---------------------------------------------------------------------------------------------- As you mentioned, getWithUndefinedOrNullCheck() will return false and doesn't modify its out-parameter. So, I think we can do early null check without creating of empty string object because of short-circuit. Do I need to modify this more?
https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/140001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2379: exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type."); On 2014/06/10 08:21:35, zino wrote: > On 2014/06/10 07:25:47, fs wrote: > > I think fixing it would reduce clutter - and in the case of the exception > > message align more to where we want to be. So I suppose the exception message > > could stay for now if there's a test for it (which I believe there is.) > > You seems to want the exception message to be modified. > Is my understanding correct? I'm fine with the exception message in its current (PS4+) form. > If so, I've already fixed since patch set 4. > > I was wondering about below your comment. > ---------------------------------------------------------------------------------------------- > controlIsNull seems redundant with passOptions.control. (Ditto for idIsNull vs. > passOptions.id.isEmpty()) > ---------------------------------------------------------------------------------------------- > As you mentioned, getWithUndefinedOrNullCheck() will return false and doesn't > modify its out-parameter. > So, I think we can do early null check without creating of empty string object > because of short-circuit. What I meant was that you could do just: options.getWithUndefinedOrNullCheck("id", passOptions.id); options.getWithUndefinedOrNullCheck("control", passOptions.control); if (passOptions.id.isEmpty() && !passOptions.control) ... which is very marginally (if at all - the controls-check is likely cheaper since it avoids coercion into a boolean) worse, and IMHO easier to read. I welcome others to chime in though.
On 2014/06/10 09:05:49, fs wrote: > which is very marginally (if at all - the controls-check is likely cheaper since > it avoids coercion into a boolean) worse, and IMHO easier to read. I welcome > others to chime in though. I got it. I agree with you. Thank you :)
Dear all, I've just uploaded a new patch set. Please take a look again. Thank you :) https://codereview.chromium.org/300223009/diff/300001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html (right): https://codereview.chromium.org/300223009/diff/300001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-hit-regions-accessibility-test.html:73: debug("Two depth containers tests."); On 2014/06/09 07:27:26, dmazzoni wrote: > nit: I'd phrase this as "Depth-two container tests" Done. https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... File Source/core/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1250: if (node()->parentElement()->isInCanvasSubtree()) { On 2014/06/09 07:27:26, dmazzoni wrote: > OK for now, but add a FIXME indicating that this is inefficient if there are a > lot of elements in the canvas. > > AXComputedObjectAttributeCache might be a good candidate for speeding this up. > Again, don't worry about fixing in this change but please make a note of it. Done. https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1264: return rect; On 2014/06/09 07:27:26, dmazzoni wrote: > If the resulting rect is still empty, fall through to the code below - a rect > somewhere in the canvas is much better than an empty rect. > Done. https://codereview.chromium.org/300223009/diff/300001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1267: // AXNodeObjects have no mechanism yet to return a size or position. On 2014/06/09 07:27:26, dmazzoni wrote: > Delete this first bit of the comment and change it to say, if this object > doesn't have an explicit element rect or one computable from its children, > return the position of the ancestor that does have... > Done. https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:27: // Offset by the canvas rect (We should take border and padding into account). On 2014/06/09 07:27:26, dmazzoni wrote: > What do you mean by "we should take border and padding into account"? > > 1. Offset by the canvas rect, taking border and padding into account > 2. Offset by the canvas rect. FIXME: we should take border and padding into > account Done. https://codereview.chromium.org/300223009/diff/300001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:36: if (!axObjectCache) On 2014/06/09 07:27:26, dmazzoni wrote: > Move this test up to the top; there's no reason to do the rest of the > calculation if there's no AXObjectCache. Done. https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.cpp (right): https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.cpp:1390: SkPath totalClipPath(path.skPath()); On 2014/06/09 11:06:27, fs wrote: > This doesn't appear to work in the general case - i.e. consider for instance a > sequence of an empty element followed by a non-empty replace-op element (or > really just a replace-op element). That may or may not be problematic in > practice, but it'd probably make sense to at least document (and possibly > assert) what isn't expected to work. Done. https://codereview.chromium.org/300223009/diff/300001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.cpp:1401: totalClipPath = clipPath; On 2014/06/09 11:06:27, fs wrote: > Since the result of this method should be the intersection between |path| and > the clip-stack, this won't do the right thing. Maybe it'd be better to simply > compute the actual clip-path, and then perform the intersection against |path|? Done.
https://codereview.chromium.org/300223009/diff/360001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/360001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388: specifiedPath.intersectPath(context->getCurrentClipPath()); The path could be "degenerate" after this operation, so I think that the bounding-box of the path needs to checked at this step.
In many cases, I think users will use non-clip mode rather than clip mode. So, I added some logic to check clip mode. Thank you. https://codereview.chromium.org/300223009/diff/360001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/360001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388: specifiedPath.intersectPath(context->getCurrentClipPath()); On 2014/06/10 11:45:15, fs wrote: > The path could be "degenerate" after this operation, so I think that the > bounding-box of the path needs to checked at this step. Done.
lgtm for accessibility https://codereview.chromium.org/300223009/diff/380001/Source/core/accessibili... File Source/core/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/300223009/diff/380001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1271: // For now, let's return the position of the ancestor that does have a position, nit: lowercase 'f' since this is continuing the sentence now.
Thank you for lgtm and I'm waiting for reviewer's response about other parts. https://codereview.chromium.org/300223009/diff/380001/Source/core/accessibili... File Source/core/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/300223009/diff/380001/Source/core/accessibili... Source/core/accessibility/AXNodeObject.cpp:1271: // For now, let's return the position of the ancestor that does have a position, On 2014/06/10 15:43:02, dmazzoni wrote: > nit: lowercase 'f' since this is continuing the sentence now. Done.
Ping all reviewers. Could you please review again (core, bindings, platform)?
LGTM for bindings/ and oilpan. https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.cpp (right): https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:57: #if ENABLE(OILPAN) Nit: You don't need to add this. https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.cpp:155: #if ENABLE(OILPAN) You don't need to add this. https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... File Source/core/html/canvas/HitRegion.h (right): https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:81: explicit HitRegionManager() { } Drop explicit. https://codereview.chromium.org/300223009/diff/400001/Source/core/html/canvas... Source/core/html/canvas/HitRegion.h:83: typedef WillBeHeapListHashSet< RefPtrWillBeMember<HitRegion> > HitRegionList; Nit: No need to add a space after '<'.
core changes look good to me, but you should probably allow junov to comment.
On 2014/06/12 08:35:33, fs wrote: > core changes look good to me, but you should probably allow junov to comment. Thanks for review and your help. I agree, I'm waiting for Justin's review. (He seems to be able to review for core/* and platform/*)
lgtm This look reasonable. I will test this code once it lands to verify the edge cases.
lgtm
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/300223009/410001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/html/canvas/CanvasRenderingContext2D.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/html/canvas/CanvasRenderingContext2D.cpp
Hunk #1 FAILED at 37.
Hunk #2 succeeded at 166 (offset -2 lines).
Hunk #3 succeeded at 1243 (offset -2 lines).
Hunk #4 succeeded at 2360 (offset 1 line).
1 out of 4 hunks FAILED -- saving rejects to file
Source/core/html/canvas/CanvasRenderingContext2D.cpp.rej
Patch: Source/core/html/canvas/CanvasRenderingContext2D.cpp
Index: Source/core/html/canvas/CanvasRenderingContext2D.cpp
diff --git a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
index
1c92df5400c91c95558a8266871c7923bbed128c..625cf00d7e23d9c0c2d17669d5c9f17abf6bbdd5
100644
--- a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
+++ b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
@@ -37,7 +37,6 @@
#include "bindings/v8/ExceptionMessages.h"
#include "bindings/v8/ExceptionState.h"
#include "bindings/v8/ExceptionStatePlaceholder.h"
-#include "core/accessibility/AXObjectCache.h"
#include "core/css/CSSFontSelector.h"
#include "core/css/parser/BisonCSSParser.h"
#include "core/css/StylePropertySet.h"
@@ -168,6 +167,7 @@ void CanvasRenderingContext2D::trace(Visitor* visitor)
#if ENABLE(OILPAN)
visitor->trace(m_stateStack);
visitor->trace(m_fetchedFonts);
+ visitor->trace(m_hitRegionManager);
#endif
CanvasRenderingContext::trace(visitor);
}
@@ -1244,6 +1244,8 @@ void CanvasRenderingContext2D::clearRect(float x, float y,
float width, float he
context->setCompositeOperation(CompositeSourceOver);
}
context->clearRect(rect);
+ if (m_hitRegionManager)
+ m_hitRegionManager->removeHitRegionsInRect(rect, state().m_transform);
if (saved)
context->restore();
@@ -2356,4 +2358,89 @@ void CanvasRenderingContext2D::drawFocusRing(const Path&
path)
didDraw(dirtyRect);
}
+void CanvasRenderingContext2D::addHitRegion(ExceptionState& exceptionState)
+{
+ addHitRegion(Dictionary(), exceptionState);
+}
+
+void CanvasRenderingContext2D::addHitRegion(const Dictionary& options,
ExceptionState& exceptionState)
+{
+ HitRegionOptions passOptions;
+
+ options.getWithUndefinedOrNullCheck("id", passOptions.id);
+ options.getWithUndefinedOrNullCheck("control", passOptions.control);
+ if (passOptions.id.isEmpty() && !passOptions.control) {
+ exceptionState.throwDOMException(NotSupportedError, "Both id and
control are null.");
+ return;
+ }
+
+ FloatRect clipBounds;
+ GraphicsContext* context = drawingContext();
+
+ if (m_path.isEmpty() || !context || !state().m_invertibleCTM
+ || !context->getTransformedClipBounds(&clipBounds)) {
+ exceptionState.throwDOMException(NotSupportedError, "The specified path
has no pixels.");
+ return;
+ }
+
+ Path specifiedPath = m_path;
+ specifiedPath.transform(state().m_transform);
+
+ if (context->isClipMode()) {
+ FloatRect specifiedBounds = specifiedPath.boundingRect();
+
+ if (!specifiedBounds.intersects(clipBounds)
+ || !specifiedPath.intersectPath(context->getCurrentClipPath())
+ || specifiedPath.isEmpty()) {
+ exceptionState.throwDOMException(NotSupportedError, "The specified
path has no pixels.");
+ return;
+ }
+ }
+
+ passOptions.path = specifiedPath;
+ addHitRegionInternal(passOptions, exceptionState);
+}
+
+void CanvasRenderingContext2D::addHitRegionInternal(const HitRegionOptions&
options, ExceptionState& exceptionState)
+{
+ if (!m_hitRegionManager)
+ m_hitRegionManager = HitRegionManager::create();
+
+ // Remove previous region (with id or control)
+ m_hitRegionManager->removeHitRegionById(options.id);
+ m_hitRegionManager->removeHitRegionByControl(options.control.get());
+
+ RefPtrWillBeRawPtr<HitRegion> hitRegion = HitRegion::create(options);
+ hitRegion->updateAccessibility(canvas());
+ m_hitRegionManager->addHitRegion(hitRegion.release());
+}
+
+void CanvasRenderingContext2D::removeHitRegion(const String& id)
+{
+ if (m_hitRegionManager)
+ m_hitRegionManager->removeHitRegionById(id);
+}
+
+void CanvasRenderingContext2D::clearHitRegions()
+{
+ if (m_hitRegionManager)
+ m_hitRegionManager->removeAllHitRegions();
+}
+
+HitRegion* CanvasRenderingContext2D::hitRegionAtPoint(const LayoutPoint& point)
+{
+ if (m_hitRegionManager)
+ return m_hitRegionManager->getHitRegionAtPoint(point);
+
+ return 0;
+}
+
+unsigned CanvasRenderingContext2D::hitRegionsCount() const
+{
+ if (m_hitRegionManager)
+ return m_hitRegionManager->getHitRegionsCount();
+
+ return 0;
+}
+
} // namespace WebCore
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/300223009/430001
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/300223009/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/300223009/470001
The CQ bit was unchecked by jinho.bang@samsung.com
On 2014/06/17 07:59:20, zino wrote: > The CQ bit was unchecked by mailto:jinho.bang@samsung.com Before landing this, we should add SK_API to SkClipStack::Element and SkClipStack::Iter for win_blink_compile_dbg. (https://codereview.chromium.org/340453003/)
adding Cary Clark, who is responsible for the geometric apis in skia.
https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || !specifiedPath.intersectPath(context->getCurrentClipPath()) I'm not sure getCurrentClipPath() is really needed. How about something like this: bool pathIsFullyClipped(SkCanvas* canvas, const SkPath& path) { if (canvas->quickReject(path)) return true; // Only needed if quickReject is too conservative canvas->save(); canvas->clipPath(path); bool isClipped = canvas->isClipEmpty(); canvas->restore(); return isClipped; }
Thank you for review and good information :) If it only has rejection case, your idea seems good to me. I'm not sure because I'm not familiar with skia side codes. But I think we still need a current clip path. If you have another good ideas, please share to me. Thank you. https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || !specifiedPath.intersectPath(context->getCurrentClipPath()) The method sets the intersection path between two path to lvalue(specifiedPath). (Note: the method doesn't return whether or not to intersect) As a result, if specifiedPath is not empty (clipPath is not empty), we should pass it and should save it to HitRegion class. Also, the path region will be used continuously while hit region exists.
https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || !specifiedPath.intersectPath(context->getCurrentClipPath()) On 2014/06/17 23:46:10, zino wrote: > The method sets the intersection path between two path to lvalue(specifiedPath). > (Note: the method doesn't return whether or not to intersect) > > As a result, if specifiedPath is not empty (clipPath is not empty), > we should pass it and should save it to HitRegion class. > Also, the path region will be used continuously while hit region exists. Why are you looking at the clipping path? Hit regions are not affected by clipping.
On 2014/06/17 23:58:58, Rik wrote: > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): > > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || > !specifiedPath.intersectPath(context->getCurrentClipPath()) > On 2014/06/17 23:46:10, zino wrote: > > The method sets the intersection path between two path to > lvalue(specifiedPath). > > (Note: the method doesn't return whether or not to intersect) > > > > As a result, if specifiedPath is not empty (clipPath is not empty), > > we should pass it and should save it to HitRegion class. > > Also, the path region will be used continuously while hit region exists. > > Why are you looking at the clipping path? Hit regions are not affected by > clipping. Please refer to the spec[1] regarding addHitRegion(). The spec says: ''' 3. Let specified pixels be the pixels contained in source path. 4. Remove from specified pixels any pixels not contained within the clipping region. ''' [1] http://www.w3.org/TR/2dcontext/#dom-context-2d-addhitregion
On 2014/06/18 00:15:29, zino wrote: > On 2014/06/17 23:58:58, Rik wrote: > > > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): > > > > > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || > > !specifiedPath.intersectPath(context->getCurrentClipPath()) > > On 2014/06/17 23:46:10, zino wrote: > > > The method sets the intersection path between two path to > > lvalue(specifiedPath). > > > (Note: the method doesn't return whether or not to intersect) > > > > > > As a result, if specifiedPath is not empty (clipPath is not empty), > > > we should pass it and should save it to HitRegion class. > > > Also, the path region will be used continuously while hit region exists. > > > > Why are you looking at the clipping path? Hit regions are not affected by > > clipping. > > Please refer to the spec[1] regarding addHitRegion(). The spec says: > ''' > 3. Let specified pixels be the pixels contained in source path. > 4. Remove from specified pixels any pixels not contained within the clipping > region. ah, you're correct. I need to update the FF code to match that :-) I assume that clipping path is the union of all the clips in the canvas state?
On 2014/06/18 07:03:32, Rik wrote: > On 2014/06/18 00:15:29, zino wrote: > > On 2014/06/17 23:58:58, Rik wrote: > > > > > > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > > > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): > > > > > > > > > https://codereview.chromium.org/300223009/diff/470001/Source/core/html/canvas... > > > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2394: || > > > !specifiedPath.intersectPath(context->getCurrentClipPath()) > > > On 2014/06/17 23:46:10, zino wrote: > > > > The method sets the intersection path between two path to > > > lvalue(specifiedPath). > > > > (Note: the method doesn't return whether or not to intersect) > > > > > > > > As a result, if specifiedPath is not empty (clipPath is not empty), > > > > we should pass it and should save it to HitRegion class. > > > > Also, the path region will be used continuously while hit region exists. > > > > > > Why are you looking at the clipping path? Hit regions are not affected by > > > clipping. > > > > Please refer to the spec[1] regarding addHitRegion(). The spec says: > > ''' > > 3. Let specified pixels be the pixels contained in source path. > > 4. Remove from specified pixels any pixels not contained within the clipping > > region. > > ah, you're correct. I need to update the FF code to match that :-) > I assume that clipping path is the union of all the clips in the canvas state? The 'clipping region' (aka 'current clipping region') is the clip path of the top-most canvas state. (What operations are required to get it will depend on the implementation, but 'union' is unlikely to be one of them =))
On 2014/06/18 07:03:32, Rik wrote: > On 2014/06/18 00:15:29, zino wrote: > > 3. Let specified pixels be the pixels contained in source path. > > 4. Remove from specified pixels any pixels not contained within the clipping > > region. > > ah, you're correct. I need to update the FF code to match that :-) Since the spec is still being worked on, is this something we can revisit? Implementation-wise this is doable in Skia, but we're somewhat hesitant to commit to the sort of API needed for this feature. I suspect this may be quite challenging to implement for other engines/graphics backends (without PathOps-like functionality).
On 2014/06/18 13:47:01, Florin Malita wrote:
> On 2014/06/18 07:03:32, Rik wrote:
> > On 2014/06/18 00:15:29, zino wrote:
> > > 3. Let specified pixels be the pixels contained in source path.
> > > 4. Remove from specified pixels any pixels not contained within the
clipping
> > > region.
> >
> > ah, you're correct. I need to update the FF code to match that :-)
>
> Since the spec is still being worked on, is this something we can revisit?
>
> Implementation-wise this is doable in Skia, but we're somewhat hesitant to
> commit to the sort of API needed for this feature. I suspect this may be quite
> challenging to implement for other engines/graphics backends (without
> PathOps-like functionality).
The challenging part is only the actual "compute intersection" bit - for
hit-testing you should be able to get away with point-in-path alone (which
necessarily won't be very performant because theoretically you could have one
clip-path per <canvas> state-stack-frame). So just dropping the requirement that
('current path' intersection 'clipping region') should be empty, possibly
replacing it w/ ('current path' is empty || 'clipping region' is empty), ought
illuminate a path out of the woods - from a spec perspective.
On 2014/06/18 14:18:51, fs wrote:
> ('current path' intersection 'clipping region') should be empty, possibly
> replacing it w/ ('current path' is empty || 'clipping region' is empty), ought
> illuminate a path out of the woods - from a spec perspective.
Sorry, I can't understand. How can we replace it?
It seems to not be able to cover the following case:
current path isn't empty and clipping region isn't empty as well but the path
doesn't intersect with the region.
On 2014/06/18 15:47:10, zino wrote:
> On 2014/06/18 14:18:51, fs wrote:
> > ('current path' intersection 'clipping region') should be empty, possibly
> > replacing it w/ ('current path' is empty || 'clipping region' is empty),
ought
> > illuminate a path out of the woods - from a spec perspective.
>
> Sorry, I can't understand. How can we replace it?
>
> It seems to not be able to cover the following case:
> current path isn't empty and clipping region isn't empty as well but the path
> doesn't intersect with the region.
Yes, that is the price that would be paid for "lowering" the implementation
complexity from the spec. (Of course just allowing both to be empty works too,
since I don't believe the hit-regions are observable anyway.)
On 2014/06/18 15:59:27, fs wrote:
> On 2014/06/18 15:47:10, zino wrote:
> > On 2014/06/18 14:18:51, fs wrote:
> > > ('current path' intersection 'clipping region') should be empty, possibly
> > > replacing it w/ ('current path' is empty || 'clipping region' is empty),
> ought
> > > illuminate a path out of the woods - from a spec perspective.
> >
> > Sorry, I can't understand. How can we replace it?
> >
> > It seems to not be able to cover the following case:
> > current path isn't empty and clipping region isn't empty as well but the
path
> > doesn't intersect with the region.
>
> Yes, that is the price that would be paid for "lowering" the implementation
> complexity from the spec. (Of course just allowing both to be empty works too,
> since I don't believe the hit-regions are observable anyway.)
Oh, I understood. You're right!!
Thank you for good information.
I'll update codes soon.
But getCurrentClipPath() is still needed.
On 2014/06/18 16:33:28, zino wrote:
> On 2014/06/18 15:59:27, fs wrote:
> > On 2014/06/18 15:47:10, zino wrote:
> > > On 2014/06/18 14:18:51, fs wrote:
> > > > ('current path' intersection 'clipping region') should be empty,
possibly
> > > > replacing it w/ ('current path' is empty || 'clipping region' is empty),
> > ought
> > > > illuminate a path out of the woods - from a spec perspective.
> > >
> > > Sorry, I can't understand. How can we replace it?
> > >
> > > It seems to not be able to cover the following case:
> > > current path isn't empty and clipping region isn't empty as well but the
> path
> > > doesn't intersect with the region.
> >
> > Yes, that is the price that would be paid for "lowering" the implementation
> > complexity from the spec. (Of course just allowing both to be empty works
too,
> > since I don't believe the hit-regions are observable anyway.)
>
> Oh, I understood. You're right!!
> Thank you for good information.
> I'll update codes soon.
You don't need to update the implementation until (and if) the spec changes.
> But getCurrentClipPath() is still needed.
Yepp.
On 2014/06/18 16:49:25, fs wrote: > > > Yes, that is the price that would be paid for "lowering" the implementation > > > complexity from the spec. (Of course just allowing both to be empty works > too, > > > since I don't believe the hit-regions are observable anyway.) > > > > Oh, I understood. You're right!! > > Thank you for good information. > > I'll update codes soon. > > You don't need to update the implementation until (and if) the spec changes. > Sorry, I was really confusing.. T_T ------------------------------------------------ if current path is empty or clipping region is empty then throw excetpion and abort the steps. else // if the path isn't empty and the region isn't empty and the path doesn't intersect then ... need intersection operation again create hit region ------------------------------------------------ In that case, as you mentioned, hit-regions can't be observable. But it may create empty hit region? (it seems to be unnecessary?)
On 2014/06/18 14:18:51, fs wrote:
...
>
> The challenging part is only the actual "compute intersection" bit - for
> hit-testing you should be able to get away with point-in-path alone (which
> necessarily won't be very performant because theoretically you could have one
> clip-path per <canvas> state-stack-frame).
You could hit test the hit region path + the path of each clip in the canvas
state. You likely still have those around.
Clip paths are mostly rectangles so that should be very fast.
> So just dropping the requirement that
> ('current path' intersection 'clipping region') should be empty, possibly
> replacing it w/ ('current path' is empty || 'clipping region' is empty), ought
> illuminate a path out of the woods - from a spec perspective.
This was something that I pointed out to Ian on WhatWG but he didn't want to
change the spec.
This requirement is not in the W3C version: a region is allows to be empty since
it might be very hard for an author to know if a region is completely clipped.
On 2014/06/18 17:04:44, zino wrote: > On 2014/06/18 16:49:25, fs wrote: > > > > Yes, that is the price that would be paid for "lowering" the > implementation > > > > complexity from the spec. (Of course just allowing both to be empty works > > too, > > > > since I don't believe the hit-regions are observable anyway.) > > > > > > Oh, I understood. You're right!! > > > Thank you for good information. > > > I'll update codes soon. > > > > You don't need to update the implementation until (and if) the spec changes. > > > > Sorry, I was really confusing.. T_T > > ------------------------------------------------ > if current path is empty or clipping region is empty then > throw excetpion and abort the steps. > else // if the path isn't empty and the region isn't empty and the path > doesn't intersect then > ... > need intersection operation again This step wouldn't be required because you could test the clip and the path separately (if you lack path-intersection). > In that case, as you mentioned, hit-regions can't be observable. > But it may create empty hit region? (it seems to be unnecessary?) Yes, there'd be a risk of keeping the additional data. It's quite the edge-case though I'd think.
On 2014/06/18 19:11:05, Rik wrote:
> On 2014/06/18 14:18:51, fs wrote:
> ...
> >
> > The challenging part is only the actual "compute intersection" bit - for
> > hit-testing you should be able to get away with point-in-path alone (which
> > necessarily won't be very performant because theoretically you could have
one
> > clip-path per <canvas> state-stack-frame).
>
> You could hit test the hit region path + the path of each clip in the canvas
> state. You likely still have those around.
Yepp, that's what I aluded to =)
> Clip paths are mostly rectangles so that should be very fast.
Yes, in the common case it'd probably work quite well.
> > So just dropping the requirement that
> > ('current path' intersection 'clipping region') should be empty, possibly
> > replacing it w/ ('current path' is empty || 'clipping region' is empty),
ought
> > illuminate a path out of the woods - from a spec perspective.
>
> This was something that I pointed out to Ian on WhatWG but he didn't want to
> change the spec.
> This requirement is not in the W3C version: a region is allows to be empty
since
> it might be very hard for an author to know if a region is completely clipped.
So addhitRegion, step 8 should not read "If the specified path has no pixels,
..." but rather ~"If the source path has no pixels, ..." ('specified path' does
not appear to be defined - there's 'specified pixels' and 'source path' so it
seems there's either a typo in there, or a missing definition.)
On 2014/06/19 07:55:24, fs wrote: > On 2014/06/18 17:04:44, zino wrote: > > On 2014/06/18 16:49:25, fs wrote: > > > > > Yes, that is the price that would be paid for "lowering" the > > implementation > > > > > complexity from the spec. (Of course just allowing both to be empty > works > > > too, > > > > > since I don't believe the hit-regions are observable anyway.) > > > > > > > > Oh, I understood. You're right!! > > > > Thank you for good information. > > > > I'll update codes soon. > > > > > > You don't need to update the implementation until (and if) the spec changes. > > > > > > > Sorry, I was really confusing.. T_T > > > > ------------------------------------------------ > > if current path is empty or clipping region is empty then > > throw excetpion and abort the steps. > > else // if the path isn't empty and the region isn't empty and the path > > doesn't intersect then > > ... > > need intersection operation again > > This step wouldn't be required because you could test the clip and the path > separately (if you lack path-intersection). > > > In that case, as you mentioned, hit-regions can't be observable. > > But it may create empty hit region? (it seems to be unnecessary?) > > Yes, there'd be a risk of keeping the additional data. It's quite the edge-case > though I'd think. Thank you for the detailed explanation. I understood it :-) (Especially, I was not sure how we can do it without the additional data)
On 2014/06/18 19:11:05, Rik wrote: > On 2014/06/18 14:18:51, fs wrote: > ... > > > > The challenging part is only the actual "compute intersection" bit - for > > hit-testing you should be able to get away with point-in-path alone (which > > necessarily won't be very performant because theoretically you could have one > > clip-path per <canvas> state-stack-frame). > > You could hit test the hit region path + the path of each clip in the canvas > state. You likely still have those around. > Clip paths are mostly rectangles so that should be very fast. I'm not sure this will work as intended: if the hit region is to be subject to clipping (as currently spec'd), then we need to capture the state of the clip stack *at the time when the hit region is installed* (the clip can obviously change after adding a hit region, and hit-testing against the current clip stack is probably not the intent).
On 2014/06/19 13:33:05, Florin Malita wrote: > On 2014/06/18 19:11:05, Rik wrote: > > On 2014/06/18 14:18:51, fs wrote: > > ... > > > > > > The challenging part is only the actual "compute intersection" bit - for > > > hit-testing you should be able to get away with point-in-path alone (which > > > necessarily won't be very performant because theoretically you could have > one > > > clip-path per <canvas> state-stack-frame). > > > > You could hit test the hit region path + the path of each clip in the canvas > > state. You likely still have those around. > > Clip paths are mostly rectangles so that should be very fast. > > I'm not sure this will work as intended: if the hit region is to be subject to > clipping (as currently spec'd), then we need to capture the state of the clip > stack *at the time when the hit region is installed* (the clip can obviously > change after adding a hit region, and hit-testing against the current clip stack > is probably not the intent). I'm not sure but I don't think we need to ensure validation of the region after adding a hit region. The path as well as the clipping region can always be changed after adding. So, I think JS authors should ensure it by themselves. (remove the region and then add a new region)
On 2014/06/19 13:33:05, Florin Malita wrote: > On 2014/06/18 19:11:05, Rik wrote: > ... > I'm not sure this will work as intended: if the hit region is to be subject to > clipping (as currently spec'd), then we need to capture the state of the clip > stack *at the time when the hit region is installed* (the clip can obviously > change after adding a hit region, and hit-testing against the current clip stack > is probably not the intent). You need to capture the current clip paths of the canvas state at the time 'addHitRegion' is called. If they are ref-counted, you can simply hold on to them. If not, you will have to make a copy.
On 2014/06/19 14:16:35, zino wrote: > > ... > > I'm not sure but I don't think we need to ensure validation of the region after > adding a hit region. > The path as well as the clipping region can always be changed after adding. > So, I think JS authors should ensure it by themselves. (remove the region and > then add a new region) I'm not sure I follow. If a hit region is clipped and then the clip is remove (ie save/restore), that region should stay clipped.
On 2014/06/19 16:37:47, Rik wrote: > I'm not sure I follow. If a hit region is clipped and then the clip is remove > (ie save/restore), that region should stay clipped. I was on the same page. Thank you.
Dear all,
I'd like to separate clipping region parts from this CL.
Other parts have already gotten LGTM.
My suggestion is:
...
if (isClipMode)
{
// FIXME: blah blah~
// crbug.com/xxxxxx
throw NotSupportedError
}
...
I think it is enough safe because the feature is already behind runtime flag.
And, If this CL would be landed, I can do next works (more tests and options)
Moreover, some people may not want to receive this mail threads no longer.
If there is no objection, I'll upload a new patch without clipping region parts
and the will request to review again. (I will also create the new CL for
clipping region parts)
What do you think of that?
+1 Not just because the clip regions is unresolved, but because landing features in small pieces is generally preferable. The reviews tend to be faster and of higher quality. The reviews involve fewer areas of the code, and therefore fewer reviewers at the same time). In general, large code reviews tend to slow down the process.
On 2014/06/20 12:28:59, junov wrote: > +1 > Not just because the clip regions is unresolved, but because landing features in > small pieces is generally preferable. The reviews tend to be faster and of > higher quality. The reviews involve fewer areas of the code, and therefore fewer > reviewers at the same time). In general, large code reviews tend to slow down > the process. I will do that. Thank you for the detailed explanation :)
Dear all, I've removed the clipping region parts from this CL. Could you please confirm again before landing this? Thank you. https://codereview.chromium.org/300223009/diff/490001/Source/core/html/canvas... File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/300223009/diff/490001/Source/core/html/canvas... Source/core/html/canvas/CanvasRenderingContext2D.cpp:2391: // FIXME: The hit regions should take clipping region into account. Remove clipping region part and leave comments. https://codereview.chromium.org/300223009/diff/490001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.h (left): https://codereview.chromium.org/300223009/diff/490001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.h:371: Remove getCurrentClipPath();
lgtm
lgtm
lgtm
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/300223009/490001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/23613)
Message was sent while issue was closed.
Change committed as 176674
Message was sent while issue was closed.
On 2014/06/21 04:25:53, I haz the power (commit-bot) wrote: > Change committed as 176674 looks like ASAN bot is unhappy with the patch. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu... worker/2 virtual/gpu/fast/canvas/canvas-hit-regions-clear-test.html crashed, (stderr lines): 04:19:33.843 8299 ================================================================= 04:19:33.843 8299 ==7==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030003051a8 at pc 0x24f0482 bp 0x7fffead3df30 sp 0x7fffead3df28 04:19:33.843 8299 READ of size 8 at 0x6030003051a8 thread T0 (content_shell) 04:19:33.843 8299 #0 0x24f0481 in WTF::ListHashSetNode<WTF::RefPtr<WebCore::HitRegion>, WTF::ListHashSetAllocator<WTF::RefPtr<WebCore::HitRegion>, 256ul> >::prev() const /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../third_party/WebKit/Source/wtf/ListHashSet.h:420:0 04:19:33.843 8299 #1 0x24ef218 in WTF::ListHashSetConstReverseIterator<WTF::ListHashSet<WTF::RefPtr<WebCore::HitRegion>, 256ul, WTF::PtrHash<WTF::RefPtr<WebCore::HitRegion> >, WTF::ListHashSetAllocator<WTF::RefPtr<WebCore::HitRegion>, 256ul> > >::operator++() /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../third_party/WebKit/Source/wtf/ListHashSet.h:615:0 04:19:33.843 8299 #2 0x24eefa7 in WebCore::HitRegionManager::removeHitRegionsInRect(WebCore::FloatRect const&, WebCore::AffineTransform const&) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../third_party/WebKit/Source/core/html/canvas/HitRegion.cpp:106:0 04:19:33.843 8299 #3 0x24db2ff in WebCore::CanvasRenderingContext2D::clearRect(float, float, float, float) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext2D.cpp:1247:0 04:19:33.843 8299 #4 0x38c677d in WebCore::CanvasRenderingContext2DV8Internal::clearRectMethod(v8::FunctionCallbackInfo<v8::Value> const&) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/gen/blink/bindings/core/v8/V8CanvasRenderingContext2D.cpp:1025:0 04:19:33.843 8299 #5 0x38a4c9c in WebCore::CanvasRenderingContext2DV8Internal::clearRectMethodCallback(v8::FunctionCallbackInfo<v8::Value> const&) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/gen/blink/bindings/core/v8/V8CanvasRenderingContext2D.cpp:1031:0 04:19:33.843 8299 #6 0x1fb7114 in v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../v8/src/arguments.cc:33:0 04:19:33.843 8299 #7 0x19ff57e in v8::internal::Object* v8::internal::HandleApiCallHelper<false>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../v8/src/builtins.cc:1208:0 04:19:33.843 8299 #8 0x19fa6e5 in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) /mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../v8/src/builtins.cc:1224:0
Message was sent while issue was closed.
On 2014/06/23 13:40:14, loislo wrote: > On 2014/06/21 04:25:53, I haz the power (commit-bot) wrote: > > Change committed as 176674 > > looks like ASAN bot is unhappy with the patch. > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu... > I'm on it. Was there a bug filed for this? |
