Bug 174539 – [iOS WK2] Presenting an action sheet on an image map prevents selection UI from updating

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/33307395>

[Wenson Hsieh]

Created attachment 315535 [details]
Patch

[Wenson Hsieh]

Created attachment 315537 [details]
Rebase on master

[Build Bot]

Comment on attachment 315537 [details]
Rebase on master

Attachment 315537 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4124752

New failing tests:
http/tests/canvas/canvas-tainted-after-draw-image.html

[Build Bot]

Created attachment 315539 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Comment on attachment 315537 [details]
Rebase on master

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

> Source/WebCore/editing/Editor.cpp:191
> +    , m_appearanceUpdatesWereEnabled(frame.selection().updateAppearanceEnabled())

This is calling an iOS-specific function, but it’s in platform-independent code. Need to either wrap it in PLATFORM(IOS) or make some bigger change.

> Source/WebCore/editing/Editor.cpp:194
> +    if (options & TemporarySelectionOptionEnableAppearanceUpdates)
> +        frame.selection().setUpdateAppearanceEnabled(true);

Ditto.

> Source/WebCore/editing/Editor.cpp:216
> +    if (m_options & TemporarySelectionOptionEnableAppearanceUpdates)
> +        m_frame.selection().setUpdateAppearanceEnabled(m_appearanceUpdatesWereEnabled);

Ditto.

> Source/WebCore/editing/Editor.h:112
> +typedef uint8_t TemporarySelectionOptions;

In new code we normally use "using" rather than "typedef".

> Source/WebCore/editing/Editor.h:115
> +    WTF_MAKE_FAST_ALLOCATED;

I don’t think we need this. We have no intention of allocating these as separate objects on the heap. I don’t want to add WTF_MAKE_FAST_ALLOCATED to every class.

> Source/WebCore/editing/Editor.h:121
> +    Frame& m_frame;

Is it safe for this to be a raw reference to a Frame rather than a Ref<Frame>? I mean it obviously is safe in the one place you are currently using it because there is a "protector" there, but what about longer term as we adopt it in more places? I suggest using Ref<Frame>.

> Source/WebCore/editing/FrameSelection.h:239
> +    bool updateAppearanceEnabled() const { return m_updateAppearanceEnabled; }

This function name sounds like a verb phrase, true if I should "update" a flag named "appearance enabled". Maybe isUpdateAppearanceEnabled()? Or maybe come up with a clearer name for this concept some day?

[Darin Adler]

Comment on attachment 315537 [details]
Rebase on master

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

>> Source/WebCore/editing/FrameSelection.h:239
>> +    bool updateAppearanceEnabled() const { return m_updateAppearanceEnabled; }
> 
> This function name sounds like a verb phrase, true if I should "update" a flag named "appearance enabled". Maybe isUpdateAppearanceEnabled()? Or maybe come up with a clearer name for this concept some day?

Sorry I meant to say:

This function name sounds like a verb phrase, a function I should call to "update" a flag named "appearance enabled". But it’s intended to be a noun or adjective phrase I think. Maybe a better name is isUpdateAppearanceEnabled()? Or maybe come up with a clearer name for this concept some day? Because what does "update appearance enabled" mean? Does it mean "appearance updating enabled"? If so, what is "appearance updating"?

[Wenson Hsieh]

Comment on attachment 315537 [details]
Rebase on master

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

>> Source/WebCore/editing/Editor.cpp:191
>> +    , m_appearanceUpdatesWereEnabled(frame.selection().updateAppearanceEnabled())
> 
> This is calling an iOS-specific function, but it’s in platform-independent code. Need to either wrap it in PLATFORM(IOS) or make some bigger change.

Yep, fixed.

>> Source/WebCore/editing/Editor.cpp:194
>> +        frame.selection().setUpdateAppearanceEnabled(true);
> 
> Ditto.

Fixed.

>> Source/WebCore/editing/Editor.cpp:216
>> +        m_frame.selection().setUpdateAppearanceEnabled(m_appearanceUpdatesWereEnabled);
> 
> Ditto.

Fixed.

>> Source/WebCore/editing/Editor.h:112
>> +typedef uint8_t TemporarySelectionOptions;
> 
> In new code we normally use "using" rather than "typedef".

👍 changed to using TemporarySelectionOptions = uint8_t;

>> Source/WebCore/editing/Editor.h:115
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> I don’t think we need this. We have no intention of allocating these as separate objects on the heap. I don’t want to add WTF_MAKE_FAST_ALLOCATED to every class.

Ok! Removed.

>> Source/WebCore/editing/Editor.h:121
>> +    Frame& m_frame;
> 
> Is it safe for this to be a raw reference to a Frame rather than a Ref<Frame>? I mean it obviously is safe in the one place you are currently using it because there is a "protector" there, but what about longer term as we adopt it in more places? I suggest using Ref<Frame>.

True -- a Ref<Frame> would be safer here than just a Frame&.

>>> Source/WebCore/editing/FrameSelection.h:239
>>> +    bool updateAppearanceEnabled() const { return m_updateAppearanceEnabled; }
>> 
>> This function name sounds like a verb phrase, true if I should "update" a flag named "appearance enabled". Maybe isUpdateAppearanceEnabled()? Or maybe come up with a clearer name for this concept some day?
> 
> Sorry I meant to say:
> 
> This function name sounds like a verb phrase, a function I should call to "update" a flag named "appearance enabled". But it’s intended to be a noun or adjective phrase I think. Maybe a better name is isUpdateAppearanceEnabled()? Or maybe come up with a clearer name for this concept some day? Because what does "update appearance enabled" mean? Does it mean "appearance updating enabled"? If so, what is "appearance updating"?

Good point, updateAppearanceEnabled() doesn't sound like the name of a const getter :| I just chose this name to match the name of the instance variable, but there's no reason it has to be that way.

For now, I changed it to isUpdateAppearanceEnabled(), but yes -- we should consider a clearer name for this concept in a future patch. Its purpose is really to determine whether we should go through the render tree to update selection data (start and end offsets, selection state, etc.) when setting the selection. On iOS, we typically bail from this codepath via an early return in FrameSelection::updateAppearance, but for some special purposes (e.g. text indicator snapshotting) we'll need to perform this work, so we temporarily toggle this switch by setting "update appearance enabled" to true.

[Wenson Hsieh]

Created attachment 315568 [details]
Fix Mac/GTK builds

[Build Bot]

Comment on attachment 315568 [details]
Fix Mac/GTK builds

Attachment 315568 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4127737

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315571 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Comment on attachment 315568 [details]
Fix Mac/GTK builds

Code looks fine to me, but I am concerned about all the failing tests.

[Wenson Hsieh]

(In reply to Darin Adler from comment #11)
> Comment on attachment 315568 [details]
> Fix Mac/GTK builds
> 
> Code looks fine to me, but I am concerned about all the failing tests.

Thanks! These EWS failures (imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html and http/tests/canvas/canvas-tainted-after-draw-image.html) don't look related, but I'll run them locally to make sure.

[Darin Adler]

I see now. I was looking at the worse run with tons of failing tests, but also clearly not your fault.

The canvas-tainted-after-draw-image.html is also failing without your patch, so clearly not caused by the patch, but needs to be fixed by someone!

[Darin Adler]

(In reply to Darin Adler from comment #13)
> The canvas-tainted-after-draw-image.html is also failing without your patch,
> so clearly not caused by the patch, but needs to be fixed by someone!

To be precise, crashing, not failing.

[WebKit Commit Bot]

Comment on attachment 315568 [details]
Fix Mac/GTK builds

Clearing flags on attachment: 315568

Committed r219541: <http://trac.webkit.org/changeset/219541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.