StorageAreaProxy::canAccessStorage should cache the permissions.
Created attachment 146015 [details] Patch
Comment on attachment 146015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146015&action=review > Source/WebKit/chromium/ChangeLog:3 > + StorageAreaProxy::canAccessStorage should cache the permissions. prefix with [chromium] please > Source/WebKit/chromium/src/StorageAreaProxy.h:31 > +#include <map> don't use stl map but <wtf/HashMap.h> > Source/WebKit/chromium/src/StorageAreaProxy.h:78 > + mutable std::map<Frame*, bool> m_canAccessStorageCache; I wonder whether we should instead add a boolean to WebFrameImpl canAccessStorage
Created attachment 146032 [details] Patch
Comment on attachment 146015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146015&action=review >> Source/WebKit/chromium/ChangeLog:3 >> + StorageAreaProxy::canAccessStorage should cache the permissions. > > prefix with [chromium] please Done >> Source/WebKit/chromium/src/StorageAreaProxy.h:31 >> +#include <map> > > don't use stl map but <wtf/HashMap.h> Removed this >> Source/WebKit/chromium/src/StorageAreaProxy.h:78 >> + mutable std::map<Frame*, bool> m_canAccessStorageCache; > > I wonder whether we should instead add a boolean to WebFrameImpl canAccessStorage Added.
this patch looks good to me, thx
Comment on attachment 146032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146032&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1408 > + m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage); is the answer ever different for local vs session storage?
Comment on attachment 146032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146032&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408 >> + m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage); > > is the answer ever different for local vs session storage? hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results
Comment on attachment 146032 [details] Patch Ahh, this was wrong, I'll fix the patch.
Comment on attachment 146032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146032&action=review >>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408 >>>> + m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage); >>> >>> is the answer ever different for local vs session storage? >> >> hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results > > hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this... class StorageAreaProxy { bool canAccess(frame) { if (frame != m_cachedCanAccessSettingForFrame) { m_cachedCanAccessSetting = get value out of frame and such; m_cachedCanAccessSettingForFrame = frame; } return m_cachedCanAccessSetting; } void* m_cachedCanAccessSettingForFrame; // the frame ptr for which we've cached the value (null if not cached) bool m_cachedCanAccessSetting; }; Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api.
(In reply to comment #10) > (From update of attachment 146032 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146032&action=review > > >>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408 > >>>> + m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage); > >>> > >>> is the answer ever different for local vs session storage? > >> > >> hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results > > > > hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results > > hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this... What if a page has multiple frames from the same origin, all using local storage? > > class StorageAreaProxy { > > bool canAccess(frame) { > if (frame != m_cachedCanAccessSettingForFrame) { > m_cachedCanAccessSetting = get value out of frame and such; > m_cachedCanAccessSettingForFrame = frame; > } > return m_cachedCanAccessSetting; > } > > void* m_cachedCanAccessSettingForFrame; // the frame ptr for which we've cached the value (null if not cached) > bool m_cachedCanAccessSetting; > }; > > Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api. The use case is displaying different UI based on whether it's sessionStorage or localStorage
> > hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this... > > What if a page has multiple frames from the same origin, all using local storage? Each document has a distinct StorageAreaProxy, actually potentially two of them, one for session storage and another for local storage. > > > > class StorageAreaProxy { > > > > bool canAccess(frame) { > > if (frame != m_cachedCanAccessSettingForFrame) { > > m_cachedCanAccessSetting = get value out of frame and such; > > m_cachedCanAccessSettingForFrame = frame; > > } > > return m_cachedCanAccessSetting; > > } > > > > void* m_cachedCanAccessSettingForFrame; // the frame ptr for which we've cached the value (null if not cached) > > bool m_cachedCanAccessSetting; > > }; > > > > Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api. > > The use case is displaying different UI based on whether it's sessionStorage or localStorage Ok, so we keep the param then.
(In reply to comment #12) > > > class StorageAreaProxy { > > > > > > bool canAccess(frame) { > > > if (frame != m_cachedCanAccessSettingForFrame) { > > > m_cachedCanAccessSetting = get value out of frame and such; > > > m_cachedCanAccessSettingForFrame = frame; > > > } > > > return m_cachedCanAccessSetting; > > > } > > > > > > void* m_cachedCanAccessSettingForFrame; // the frame ptr for which we've cached the value (null if not cached) > > > bool m_cachedCanAccessSetting; > > > }; If you like, I can work this into another small performance related change to this class thats in the works... https://chromiumcodereview.appspot.com/10539097/
Created attachment 147699 [details] tlc
Created attachment 147701 [details] tlc added missing platform/ChangeLog diffs to the patch
Created attachment 147899 [details] tlc not sure why the patch didn't apply, reupping to run it by the ews bots again
> not sure why the patch didn't apply, reupping to run it by the ews bots again i think that may have to do with line endings fyi, the chrome-side can be seen here, https://chromiumcodereview.appspot.com/10533093/
Created attachment 148945 [details] tlc
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 148945 [details] tlc API change LGTM
(In reply to comment #20) > (From update of attachment 148945 [details]) > API change LGTM jochen, can you take a look at the .cpp parts of this?
(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 148945 [details] [details]) > > API change LGTM > > jochen, can you take a look at the .cpp parts of this? I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ?
> I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ? It sure should break that test! Probably 'passing' because of this test expectation... BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT
(In reply to comment #23) > > I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ? > > It sure should break that test! Probably 'passing' because of this test expectation... > > BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT ouch Anyway, the question is, should the cache be invalidated by the embedder? Alternatively, we could update the test to always block storage.
(In reply to comment #24) > (In reply to comment #23) > > > I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ? > > > > It sure should break that test! Probably 'passing' because of this test expectation... > > > > BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT > > ouch > > Anyway, the question is, should the cache be invalidated by the embedder? Alternatively, we could update the test to always block storage. I'll poke at the test to setup to block access prior to checking and have that test just check the blocked condition.
I've unskipped the test meanwhile. Can you update the test so it still works with this patch in?
Comment on attachment 148945 [details] tlc Clearing flags.