Combine the Telephone and Selection overlay controllers, updating UI behavior <rdar://problem/16874568>
Created attachment 234102 [details] Patch v1
Attachment 234102 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:55: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 234102 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=234102&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:527 > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) Isn't this technically an ||? We need the services overlay if we have one or the other :D > Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:47 > +class ServicesOverlayController : public RefCounted<ServicesOverlayController>, private PageOverlay::Client { Does this need to be refcounted? Doesn't the WebPage just own one? > Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:79 > + RefPtr<WebPage> m_webPage; I don't see how this ref is ever dropped. > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:30 > + extra space here
(In reply to comment #3) > (From update of attachment 234102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234102&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:527 > > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) > > Isn't this technically an ||? We need the services overlay if we have one or the other :D That configuration doesn't exist and I don't think it's worth supporting until it does. Such a config would certainly not build, for example, nor do I intend to make it build! :) > > Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:47 > > +class ServicesOverlayController : public RefCounted<ServicesOverlayController>, private PageOverlay::Client { > > Does this need to be refcounted? Doesn't the WebPage just own one? Probably. > > Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:79 > > + RefPtr<WebPage> m_webPage; > > I don't see how this ref is ever dropped. Hmmm wasn't a problem before, but we do far less "destroyOverlay" in this version. s/far less/none/ > > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:30 > > + > > extra space here New one coming.
Created attachment 234112 [details] Patch v2
Comment on attachment 234112 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=234112&action=review EWS was unable to apply the patch. I tried to review but there was a lot that I did not understand. > Source/WebCore/dom/Range.cpp:1973 > + if (this == &other) > + return true; Seems a waste to do this operation since the next check will get this. > Source/WebCore/dom/Range.cpp:1976 > + if (startPosition() == other.startPosition() && endPosition() == other.endPosition()) > + return true; Is this an important optimization? > Source/WebCore/dom/Range.cpp:1978 > + short startToStart = compareBoundaryPoints(Range::START_TO_START, &other, ASSERT_NO_EXCEPTION); Why can you assert there is no exception here? What if one range or the other is detached? Is this function legal to call only on attached ranges? What about if the two ranges are in two different documents. > Source/WebCore/dom/Range.cpp:1981 > + return startToStart <= 0 && endToEnd >= 0; Why not return early if the START_TO_START check fails? > Source/WebCore/dom/Range.h:164 > + bool contains(const Range&) const; I’m not 100% sure it’s a good idea to add this as a member function rather than a free function. > Source/WebKit2/WebProcess/WebPage/WebPage.h:187 > -#if ENABLE(TELEPHONE_NUMBER_DETECTION) > -class TelephoneNumberOverlayController; > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) > +class ServicesOverlayController; > #endif Generally I think it’s overkill to put forward declarations inside #if. I think we can include them unconditionally. Does the above need to be || rather than &&? > Source/WebKit2/WebProcess/WebPage/WebPage.h:1270 > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) Do you mean || rather than &&? > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:242 > + CGRect cgRects[] = { (CGRect)rect }; Why not just use a single rect for the local and take a pointer to it? That should work just as well as an array of size 1 and eliminates the need for the C-style type cast. > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:269 > + graphicsContext.scale(FloatSize(1, -1)); > + graphicsContext.translate(FloatSize(0, -highlightBoundingRect.size.height)); Sad that this flipping code is needed.
Created attachment 234122 [details] Patch v3
Nevermind v3 - Didn't see that Darin had commented.
(In reply to comment #6) > (From update of attachment 234112 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234112&action=review > > Source/WebCore/dom/Range.cpp:1973 > > + if (this == &other) > > + return true; > > Seems a waste to do this operation since the next check will get this. Okay. > > > Source/WebCore/dom/Range.cpp:1976 > > + if (startPosition() == other.startPosition() && endPosition() == other.endPosition()) > > + return true; > > Is this an important optimization? Maybe not. > > > Source/WebCore/dom/Range.cpp:1978 > > + short startToStart = compareBoundaryPoints(Range::START_TO_START, &other, ASSERT_NO_EXCEPTION); > > Why can you assert there is no exception here? What if one range or the other is detached? Is this function legal to call only on attached ranges? What about if the two ranges are in two different documents. These various Range comparison functions all seem to follow different rules. I'll add in the Document check. > > > Source/WebCore/dom/Range.cpp:1981 > > + return startToStart <= 0 && endToEnd >= 0; > > Why not return early if the START_TO_START check fails? Sounds good. > > > Source/WebCore/dom/Range.h:164 > > + bool contains(const Range&) const; > > I’m not 100% sure it’s a good idea to add this as a member function rather than a free function. This isn't the first time I've seen resistance adding a Range comparator function, but this one makes sense to me. The other two comparators - "areRangesEqual" and "rangesOverlap" make sense as operators applied to any two ranges in any order. Doing the same with contains() doesn't read as clearly: Range a, b; return contains(a, b); // What does this line of code actually mean? Asking one range "Do you contain this other range" makes more sense as a sentence: Range a, b; return a.contains(b); // This line of code is pretty obvious > > > Source/WebKit2/WebProcess/WebPage/WebPage.h:187 > > -#if ENABLE(TELEPHONE_NUMBER_DETECTION) > > -class TelephoneNumberOverlayController; > > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) > > +class ServicesOverlayController; > > #endif > > Does the above need to be || rather than &&? > > > Source/WebKit2/WebProcess/WebPage/WebPage.h:1270 > > +#if ENABLE(SERVICE_CONTROLS) && ENABLE(TELEPHONE_NUMBER_DETECTION) > > Do you mean || rather than &&? I definitely meant &&, as mentioned to Tim earlier. Having only one of these enabled is already relatively unsupported and will result in a broken build. After this patch, it's even worse for those who'd try to enable only one. But since you both called it out, I will relent. Nobody has that config anyways. > > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:242 > > + CGRect cgRects[] = { (CGRect)rect }; > > Why not just use a single rect for the local and take a pointer to it? That should work just as well as an array of size 1 and eliminates the need for the C-style type cast. Sounds fine. > > Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:269 > > + graphicsContext.scale(FloatSize(1, -1)); > > + graphicsContext.translate(FloatSize(0, -highlightBoundingRect.size.height)); > > Sad that this flipping code is needed. No argument here.
Created attachment 234128 [details] Patch v4
Comment on attachment 234128 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=234128&action=review > Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h:47 > + ServicesOverlayController(WebPage*); argument should be a reference :D
(In reply to comment #9) > > Do you mean || rather than &&? > > I definitely meant &&, as mentioned to Tim earlier. > > Having only one of these enabled is already relatively unsupported and will result in a broken build. After this patch, it's even worse for those who'd try to enable only one. > > But since you both called it out, I will relent. Nobody has that config anyways. Thank you for relenting :) I think it's better if it says the right thing, so that if we ever *do* need to make that configuration work, it will be more obvious what's up. And like you say, nobody has that configuration so it doesn't matter right now!
http://trac.webkit.org/changeset/170640
(In reply to comment #13) > http://trac.webkit.org/changeset/170640 http://trac.webkit.org/changeset/170641 build fix followup
http://trac.webkit.org/changeset/170652 was needed to fix this on iOS. I'm not sure the PLATFORM(MAC) ifdefs will make correct behavior, but they were needed to make it link.