Summary: | Reduce duplicated code in WebPageProxy | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||
Component: | New Bugs | Assignee: | Gustavo Noronha (kov) <gustavo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2013-10-02 13:07:30 PDT
Created attachment 213188 [details]
Patch
Comment on attachment 213188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213188&action=review Patch seems like a good idea, but there is at least one small error, and a few questions. Not sure that “reset state” is a great name for this new function, kind of vague, but I can’t quickly think of a better one. > Source/WebKit2/UIProcess/WebPageProxy.cpp:-555 > - if (m_inspector) { The code in resetState does not do this null check. What guarantees this won’t be null? > Source/WebKit2/UIProcess/WebPageProxy.cpp:-562 > - if (m_fullScreenManager) { The code in resetState does not do this null check. What guarantees this won’t be null? > Source/WebKit2/UIProcess/WebPageProxy.cpp:-605 > - invalidateCallbackMap(m_imageCallbacks); This line of code was lost. I don’t see it in resetState. > Source/WebKit2/UIProcess/WebPageProxy.cpp:-618 > - Vector<WebEditCommandProxy*> editCommandVector; > - copyToVector(m_editCommandSet, editCommandVector); > - m_editCommandSet.clear(); > - for (size_t i = 0, size = editCommandVector.size(); i < size; ++i) > - editCommandVector[i]->invalidate(); The resetState function calls m_pageClient->clearAllEditCommands(), but this code did not. Why is that change in behavior OK? Created attachment 213247 [details]
Patch
Thanks for the review! I'm not too fond of the resetState name myself, but couldn't think of anything better. All of your other comments were mistakes on my part, all fixed with this update.
Comment on attachment 213247 [details] Patch Clearing flags on attachment: 213247 Committed r157064: <http://trac.webkit.org/changeset/157064> All reviewed patches have been landed. Closing bug. |