Bug 115665 – [WK2][Mac] Pass information about open pages to LaunchServices

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

<rdar://problem/13479806>

[Alexey Proskuryakov]

Created attachment 200727 [details]
proposed patch

[Alexey Proskuryakov]

Created attachment 200733 [details]
with a little more C++11 goodness

[Darin Adler]

Comment on attachment 200733 [details]
with a little more C++11 goodness

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

r=me even without the changes I suggest (or allude to)

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:231
> +    for (const auto& iter: m_pageMap) {

We don’t normally call iterators “iter”; for whatever reason we have used “it” for this for a long time. Maybe iter is better? Or maybe we need a word to use in cases like this.

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:241
> +        if (!mainFrameOrigin->isUnique())
> +            mainFrameOriginString = mainFrameOrigin->toRawString();
> +        else
> +            mainFrameOriginString = KURL(KURL(), mainFrame->url()).protocol() + ':';

I’m surprised there isn’t a function somewhere that encapsulates this operation. Is this really an unusual requirement, this need to hide unique origins and reveal non-unique ones?

I also think this needs a why comment. It’s not obvious why uniqueness leads to a need to obscure the origin and just reveal the protocol.

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:242
> +        CFArrayAppendValue(activePageURLs.get(), userVisibleString([NSURL URLWithString:mainFrameOriginString]));

It seems so awkward to have to round trip through NSURL back to NSString to do this. Again, maybe a “why” comment explaining how this is helpful or why it‘s required would be helpful.

[Alexey Proskuryakov]

(In reply to comment #3)

> We don’t normally call iterators “iter”; for whatever reason we have used “it” for this for a long time. Maybe iter is better? Or maybe we need a word to use in cases like this.

We have a number of "iters" in code base (139 results in 57 files), and I think that I've been always naming them like that. I guess the C++11 syntax made it more noticeable!

> I’m surprised there isn’t a function somewhere that encapsulates this operation. Is this really an unusual requirement, this need to hide unique origins and reveal non-unique ones?
> 
> I also think this needs a why comment. It’s not obvious why uniqueness leads to a need to obscure the origin and just reveal the protocol.

Added a comment. What actually happens is that unique origins are indistinguishable by design, even toRawString does not necessarily work.

I do not think that there is an existing function that builds such a string for presentation to user. Another way to implement this would be to remove path, fragment and credentials from a URL, not using SecurityOrigin code at all. I think that these are equally unwieldy.

> It seems so awkward to have to round trip through NSURL back to NSString to do this. Again, maybe a “why” comment explaining how this is helpful or why it‘s required would be helpful.

The only reason I did it this way is that WebCore only exposes a Foundation-based interface for userVisibleString().

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r149646>.

[Alexey Proskuryakov]

*** Bug 115496 has been marked as a duplicate of this bug. ***