Bug 34705 - [BREWMP] Add a function to create a BREW instance without local variable declarations.
Summary: [BREWMP] Add a function to create a BREW instance without local variable decl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33564
  Show dependency treegraph
 
Reported: 2010-02-08 08:02 PST by Kwang Yul Seo
Modified: 2010-03-15 21:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.05 KB, patch)
2010-02-08 08:08 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2010-02-08 09:34 PST, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2010-03-07 01:34 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-02-08 08:02:50 PST
It is too verbose to create a BREW instance. For example, to create a IFileMgr instance, it takes three lines:

IFileMgr* fileMgr;
IShell* shell = reinterpret_cast<AEEApplet*>(GETAPPINSTANCE())->m_pIShell;
ISHELL_CreateInstance(shell, AEECLSID_FILEMGR, reinterpret_cast<void**>(&fileMgr));

This pattern is repeated all over the code, so we need a shortcut here. Add a template function to create a BREW instance in one line.

IFileMgr* fileMgr = createInstanceBrew<IFileMgr>(AEECLSID_FILEMGR);


This can be nicely combined with OwnPtrBrew.

OwnPtrBrew<IFileMgr> fileMgr(createInstanceBrew<IFileMgr>(AEECLSID_FILEMGR));
Comment 1 Kwang Yul Seo 2010-02-08 08:08:31 PST
Created attachment 48333 [details]
Patch

ShellUtilBrew.h is currently under wtf/brew, but I am not sure if this is the right place to put platform-specific utility functions.
Comment 2 Darin Adler 2010-02-08 09:13:06 PST
(In reply to comment #1)
> ShellUtilBrew.h is currently under wtf/brew, but I am not sure if this is the
> right place to put platform-specific utility functions.

It’s the right directory for low-level functions of this type. But “utility” is not a word we ever use, and we certainly frown on “Util” in a file name.
Comment 3 Kwang Yul Seo 2010-02-08 09:34:35 PST
Created attachment 48338 [details]
Patch

Remove Util from the filename and prefix the function with shell

createInstanceBrew -> shellCreateInstanceBrew
Comment 4 Eric Seidel (no email) 2010-02-08 19:20:10 PST
Should this return a PassOwnPtrBrew?
Comment 5 Kwang Yul Seo 2010-02-08 19:46:50 PST
(In reply to comment #4)
> Should this return a PassOwnPtrBrew?

These is no PassOwnPtrBrew class. I just wanted to simplify the creation of a BREW instance. As OwnPtrBrew takes a raw pointer in the constructor, we can use an idiom like the following:

OwnPtrBrew<IFileMgr> fileMgr(createInstanceBrew<IFileMgr>(AEECLSID_FILEMGR));

Do you suggest that this should return a PassOwnPtrBrew?
Comment 6 Adam Barth 2010-02-09 12:40:40 PST
> Do you suggest that this should return a PassOwnPtrBrew?

In general, createMumble methods return a PassFooBar object to ensure we don't leak the allocated memory.
Comment 7 Eric Seidel (no email) 2010-02-17 15:54:47 PST
Comment on attachment 48338 [details]
Patch

Why would this be part of WTF?  This seems like port-specific goop.

We don't need to have PassOwnPtrBrew to approve this, but eventually it would be nice to have such a class to make leaking memory returned from this function impossible.
Comment 8 Kwang Yul Seo 2010-03-07 01:34:28 PST
Created attachment 50169 [details]
Patch

Make the function return PassOwnPtr. This prevents memory leaking.
Comment 9 Eric Seidel (no email) 2010-03-15 16:21:30 PDT
Comment on attachment 50169 [details]
Patch

LGTM.
Comment 10 WebKit Commit Bot 2010-03-15 21:07:06 PDT
Comment on attachment 50169 [details]
Patch

Clearing flags on attachment: 50169

Committed r56031: <http://trac.webkit.org/changeset/56031>
Comment 11 WebKit Commit Bot 2010-03-15 21:07:13 PDT
All reviewed patches have been landed.  Closing bug.