Plug-ins that are clicked by the user should start automatically the next time they are loaded.
<rdar://problem/12746483>
Created attachment 176092 [details] Patch
Comment on attachment 176092 [details] Patch Attachment 176092 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14991730
Created attachment 176207 [details] Patch
Comment on attachment 176207 [details] Patch Attachment 176207 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15016033
Comment on attachment 176207 [details] Patch Attachment 176207 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15018100
Created attachment 176292 [details] Patch
Comment on attachment 176292 [details] Patch Attachment 176292 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15015168
Created attachment 176318 [details] Patch
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.
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.
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.
Created attachment 176354 [details] Patch
Comment on attachment 176354 [details] Patch Attachment 176354 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15018211
Comment on attachment 176354 [details] Patch Attachment 176354 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15000827
Created attachment 176360 [details] Patch
Created attachment 176388 [details] Patch
Created attachment 176817 [details] Patch
Created attachment 178185 [details] Patch
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.
Created attachment 178649 [details] Patch
Committed r137230: <http://trac.webkit.org/changeset/137230>