[webkit-reviews] review granted: [Bug 115665] [WK2][Mac] Pass information about open pages to LaunchServices : [Attachment 200733] with a little more C++11 goodness

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 115665: [WK2][Mac] Pass information about open pages to LaunchServices
https://bugs.webkit.org/show_bug.cgi?id=115665

Attachment 200733: with a little more C++11 goodness
https://bugs.webkit.org/attachment.cgi?id=200733&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.