Bug 134461 – Combine the TelephoneNumber and Selection overlay controllers, updating UI behavior

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Combine the Telephone and Selection overlay controllers, updating UI behavior

<rdar://problem/16874568>

[Brady Eidson]

Created attachment 234102 [details]
Patch v1

[WebKit Commit Bot]

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.

[Tim Horton]

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

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 234112 [details]
Patch v2

[Darin Adler]

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.

[Brady Eidson]

Created attachment 234122 [details]
Patch v3

[Brady Eidson]

Nevermind v3 - Didn't see that Darin had commented.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 234128 [details]
Patch v4

[Tim Horton]

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

[Tim Horton]

(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!

[Brady Eidson]

http://trac.webkit.org/changeset/170640

[Brady Eidson]

(In reply to comment #13)
> http://trac.webkit.org/changeset/170640

http://trac.webkit.org/changeset/170641 build fix followup

[Alex Christensen]

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.