Bug 100850

Created attachment 171638 [details] Patch

Hi Pavel! I lied. Splitting this out from 100650 makes things clearer. Mind having a quick look?

Created attachment 171644 [details] Rebased.

Comment on attachment 171644 [details] Rebased. Attachment 171644 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14677165

Comment on attachment 171644 [details] Rebased. Attachment 171644 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14661235

Created attachment 171651 [details] *ahem*

Comment on attachment 171651 [details] *ahem* Clearing flags on attachment: 171651 Committed r133053: <http://trac.webkit.org/changeset/133053>

All reviewed patches have been landed. Closing bug.

There is a WebKit design question here, that this change touches on. How many different commonly used functions do we reflect on core objects like Document and, say, Frame rather than leaving them out on the more specific objects at the periphery? There was a time where all the functions were on Document and Frame, making those sort of “mega classes”. Having those important helpful functions there does make it easier to correctly call the functions, but the size and complexity of the classes was a problem before. This patch doesn’t add a new function to Document, but to me it brings that basic design issue to mind. There’s no question in my mind that it’s easier to write code that deals directly with the more familiar objects such as Document rather than having to dig through pointers that might be null to get to the more special purpose ones, but I am not sure how scalable this approach is. There’s a similar issue with almost every Page function. There could be a Document or Frame function that calls through to Page and does the often-needed null check. My design question is this: What is it about the addConsoleMessage function that makes it important enough that it gets a coveted spot in the Document class? How can that help us decide when to do this and when to not do it in the future?

> My design question is this: What is it about the addConsoleMessage function that makes it important enough that it gets a coveted spot in the Document class? That's a good question. I've generally been in favor of removing these sorts of functions if they are just thunks. For example, we used to follow the pattern that code outside of Chrome.cpp should talk directly to ChromeClient, but now we let code call chrome()->client() functions directly, letting us remove a bunch of thunks from Chrome.cpp. In this case Document::addMessage does more work than just thunking to domWindow()->console(): http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L4873 Now, some of that work (like checking which thread we're on) isn't needed for all callers (because many callers know they're already on the main thread), but the null checks are useful. For example, https://bugs.webkit.org/attachment.cgi?id=169966&action=review is a case where we crashed because of a missing null check on Console. That case is an obscure race that involves a worker thread shutting down at just the right moment in the middle of navigation. It's not clear to me how many callers of addMessage need to worry about such things, but you can imagine that we might try to log console messages in the middle of disruptive changes, like navigating the frame. > How can that help us decide when to do this and when to not do it in the future? I guess I'm in favor of removing these sorts of functions when they're easy to inline into their callers and when its likely that future code will do the right thing. In this case, it seems like many callers will omit the Console null check, rightly or wrongly. Given that this isn't performance sensitive code, I'd rather have these callers go through this common point where we can remember to null check Console and have a test that reminds us why we do.

(In reply to comment #10) > In this case, it seems like many callers will omit the Console null check, rightly or wrongly. Sure, makes sense. I think almost all calls Page functions have the same issue, yet I don’t want to reflect all of them on Document or Frame, so I’m not sure how to apply this insight more widely across the project.