<rdar://problem/33307395>
Created attachment 315535 [details] Patch
Created attachment 315537 [details] Rebase on master
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
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
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?
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"?
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.
Created attachment 315568 [details] Fix Mac/GTK builds
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
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
Comment on attachment 315568 [details] Fix Mac/GTK builds Code looks fine to me, but I am concerned about all the failing tests.
(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.
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!
(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.
Comment on attachment 315568 [details] Fix Mac/GTK builds Clearing flags on attachment: 315568 Committed r219541: <http://trac.webkit.org/changeset/219541>
All reviewed patches have been landed. Closing bug.