Bug 103206 – Keep track of plug-in snapshots clicked by user

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

Plug-ins that are clicked by the user should start automatically the next time they are loaded.

[Jon Lee]

<rdar://problem/12746483>

[Jon Lee]

Created attachment 176092 [details]
Patch

[EFL EWS Bot]

Comment on attachment 176092 [details]
Patch

Attachment 176092 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14991730

[Jon Lee]

Created attachment 176207 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 176207 [details]
Patch

Attachment 176207 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15016033

[WebKit Review Bot]

Comment on attachment 176207 [details]
Patch

Attachment 176207 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15018100

[Jon Lee]

Created attachment 176292 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 176292 [details]
Patch

Attachment 176292 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15015168

[Jon Lee]

Created attachment 176318 [details]
Patch

[Darin Adler]

Comment on attachment 176292 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:13
> +        (PlatformStrategiesEfl):

Should remove bogus lines like this from change logs.

> Source/WebKit/gtk/ChangeLog:13
> +        (PlatformStrategiesGtk):

And this.

> Source/WebKit/mac/ChangeLog:10
> +        (WebPlatformStrategies):

And this.

> Source/WebKit/qt/ChangeLog:13
> +        (PlatformStrategiesQt):

And this.

> Source/WebKit/win/ChangeLog:13
> +        (WebPlatformStrategies):

And this.

> Source/WebKit/wince/ChangeLog:13
> +        (PlatformStrategiesWinCE):

And this.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:295
> +    m_plugInOriginHash = plugInOriginHash(document()->page(), url);

This assumes that page is non-null.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:296
> +    if (document()->page()

Then this checks that page is non-null. Too late by now because plugInOriginHash would have crashed.

> Source/WebCore/platform/PlugInOriginHash.cpp:38
> +static ALWAYS_INLINE PlugInOriginHash plugInOriginHashInline(const CharacterType* origin, unsigned length)

Why is ALWAYS_INLINE needed rather than just inline?

> Source/WebCore/platform/PlugInOriginHash.cpp:40
> +    return AlreadyHashed::avoidDeletedValue(StringHasher::computeHash(origin, length));

I think this should be a case folding hash.

This returns a 64-bit type, but I believe StringHasher returns only a 32-bit value.

> Source/WebCore/platform/PlugInOriginHash.cpp:45
> +    const KURL& mainFrameBaseURL = page->mainFrame()->document()->baseURL();

This local variable is not needed. The code reads more clearly without it.

> Source/WebCore/platform/PlugInOriginHash.cpp:50
> +    StringBuilder builder;
> +    builder.append(mainFrameBaseURL.host());
> +    builder.append(srcURL.host());
> +
> +    String hashString = builder.toString();

Building a concatenated string is not an efficient way to hash the two strings because of the additional memory allocation needed. There are alternate approaches that don’t require memory allocation or that don’t require copying all the characters. It’s a little strange to go to the trouble of making an optimized 8-bit code path when the performance cost of building the string in the first place is pretty high.

Normally we’d include a separator between the two strings to get hashed in the right way.

> Source/WebCore/platform/PlugInOriginHash.cpp:51
> +    ASSERT(hashString.length());

What guarantees the URLs have non-empty host strings? What if they are both file URLs?

> Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp:161
> +    // FIXME: Implement.

Is it really right to say we need these implemented on every platform? Not every platform will necessarily want this feature. It’s be nice to not put all these FIXMEs in, since there’s nothing to fix unless someone wants this feature to work.

> Source/WebKit2/Shared/Plugins/PlugInAutoStartTable.h:35
> +typedef HashSet<WebCore::PlugInOriginHash> PlugInAutoStartTable;

I’m not sure it’s worth having a while header file just for a HashSet typedef. We could just repeat the type twice in the two file it’s used in.

[Michael Saboff]

Comment on attachment 176318 [details]
Patch

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

> Source/WebCore/platform/PlugInOriginHash.cpp:54
> +    return plugInOriginHashInline(hashString.characters(), hashString.length());

Use characters16() since we know the string is 16 bits at this point.

[Jon Lee]

Comment on attachment 176292 [details]
Patch

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

>> Source/WebKit/efl/ChangeLog:13
>> +        (PlatformStrategiesEfl):
> 
> Should remove bogus lines like this from change logs.

I've removed all of these.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:295
>> +    m_plugInOriginHash = plugInOriginHash(document()->page(), url);
> 
> This assumes that page is non-null.

Added a check for page before this assignment...

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:296
>> +    if (document()->page()
> 
> Then this checks that page is non-null. Too late by now because plugInOriginHash would have crashed.

... and removed this check here.

>> Source/WebCore/platform/PlugInOriginHash.cpp:38
>> +static ALWAYS_INLINE PlugInOriginHash plugInOriginHashInline(const CharacterType* origin, unsigned length)
> 
> Why is ALWAYS_INLINE needed rather than just inline?

I took this from VisitedLinkHash, but after talking with Michael about this realized it was unnecessary. Switched to inline.

>> Source/WebCore/platform/PlugInOriginHash.cpp:40
>> +    return AlreadyHashed::avoidDeletedValue(StringHasher::computeHash(origin, length));
> 
> I think this should be a case folding hash.
> 
> This returns a 64-bit type, but I believe StringHasher returns only a 32-bit value.

Yes. A different scheme will be used (see below).

>> Source/WebCore/platform/PlugInOriginHash.cpp:45
>> +    const KURL& mainFrameBaseURL = page->mainFrame()->document()->baseURL();
> 
> This local variable is not needed. The code reads more clearly without it.

Done.

>> Source/WebCore/platform/PlugInOriginHash.cpp:50
>> +    String hashString = builder.toString();
> 
> Building a concatenated string is not an efficient way to hash the two strings because of the additional memory allocation needed. There are alternate approaches that don’t require memory allocation or that don’t require copying all the characters. It’s a little strange to go to the trouble of making an optimized 8-bit code path when the performance cost of building the string in the first place is pretty high.
> 
> Normally we’d include a separator between the two strings to get hashed in the right way.

We can expand the hash space by instead hashing the two origins separately and then concatenating them into a 64-bit hash. We avoid the allocation that way, also.

>> Source/WebCore/platform/PlugInOriginHash.cpp:51
>> +    ASSERT(hashString.length());
> 
> What guarantees the URLs have non-empty host strings? What if they are both file URLs?

I'll add a check for an empty string, and substitute "localhost" in that case.

>> Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp:161
>> +    // FIXME: Implement.
> 
> Is it really right to say we need these implemented on every platform? Not every platform will necessarily want this feature. It’s be nice to not put all these FIXMEs in, since there’s nothing to fix unless someone wants this feature to work.

I've remove them.

>> Source/WebKit2/Shared/Plugins/PlugInAutoStartTable.h:35
>> +typedef HashSet<WebCore::PlugInOriginHash> PlugInAutoStartTable;
> 
> I’m not sure it’s worth having a while header file just for a HashSet typedef. We could just repeat the type twice in the two file it’s used in.

I agree, but this was included in anticipation of switching to a shared memory model instead of a copied model, like the visited link table.

[Jon Lee]

Created attachment 176354 [details]
Patch

[Early Warning System Bot]

Comment on attachment 176354 [details]
Patch

Attachment 176354 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15018211

[Early Warning System Bot]

Comment on attachment 176354 [details]
Patch

Attachment 176354 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15000827

[Jon Lee]

Created attachment 176360 [details]
Patch

[Jon Lee]

Created attachment 176388 [details]
Patch

[Jon Lee]

Created attachment 176817 [details]
Patch

[Jon Lee]

Created attachment 178185 [details]
Patch

[WebKit Review Bot]

Attachment 178185 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/Page.h:175:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WebCore/page/Page.h:255:  The parameter name "volume" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Lee]

Created attachment 178649 [details]
Patch

[Jon Lee]

Committed r137230: <http://trac.webkit.org/changeset/137230>