Introduce a helper for OPENFILENAME struct to make certain logic testable.

Introduce a helper for OPENFILENAME struct to make certain logic testable.

BUG=73098
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287048

[erikwright (departed), 2014-07-31 15:07:34]

sky: PTAL. This logic was untested. Now it is extracted for testability. I will
be adding other methods here as I continue my work in
http://crrev.com/419523006/

[sky, 2014-07-31 16:00:56]

If this is only used shell_dialogs, can you leave the code there? Also, make
sure you update gn files too.

[erikwright (departed), 2014-07-31 16:05:45]

In http://crrev.com/419523006 it will also be used from chrome/utility and
chrome/browser.

Is ui/base/win right in that case? Or still move to shell_dialogs?


On Thu, Jul 31, 2014 at 12:00 PM, <sky@chromium.org> wrote:

> If this is only used shell_dialogs, can you leave the code there? Also,
> make
> sure you update gn files too.
>
> https://codereview.chromium.org/426673003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-07-31 16:27:32]

I didn't realize you wanted to use it from multiple places. Keeping here is fine
then.

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
File ui/base/win/open_file_name_win.h (right):

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
ui/base/win/open_file_name_win.h:43: void GetResult(base::FilePath* directory,
const FilePath&? Also, is there a reason you separate out the directory and file
name? Seems easier to not have to combine the paths (for example,
GetSingleResult doesn't separate out the directory from the name).

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
ui/base/win/open_file_name_win.h:47: OPENFILENAME* Get();
nit: inline this and name it name it to match field name.

[erikwright (departed), 2014-07-31 16:39:29]

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
File ui/base/win/open_file_name_win.h (right):

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
ui/base/win/open_file_name_win.h:43: void GetResult(base::FilePath* directory,
On 2014/07/31 16:27:31, sky wrote:
> const FilePath&?

Both parameters are out-parameters, unless this is your suggested return type
(see below).

> Also, is there a reason you separate out the directory and file
> name? Seems easier to not have to combine the paths (for example,
> GetSingleResult doesn't separate out the directory from the name).

In the follow-up CL you will see that I use this to marshal the results across
IPC. If I retrieve a vector of absolute file paths I need to do checks to make
sure that they are all in the same directory and have failure cases otherwise
(in order to re-serialize the result back into an OPENFILENAME).

While it might be better to just keep the result in some Chrome-specific
datastructure and not worry about this constraint of OPENFILENAME, at the moment
I can't do that. First and foremost because I want to maintain API compatibility
so that I can A/B test the utility-process based implementation against
::GetOpenFileName to measure its impact on stability.

[sky, 2014-07-31 19:38:56]

Ok, LGTM

[erikwright (departed), 2014-07-31 19:42:27]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-07-31 19:44:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/426673003/60001

[commit-bot: I haz the power, 2014-08-01 00:16:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikwright (departed), 2014-08-01 04:24:14]

The CQ bit was unchecked by erikwright@chromium.org

[erikwright (departed), 2014-08-01 13:11:32]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-08-01 13:14:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/426673003/100001

[erikwright (departed), 2014-08-01 13:26:09]

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
File ui/base/win/open_file_name_win.h (right):

https://codereview.chromium.org/426673003/diff/20001/ui/base/win/open_file_na...
ui/base/win/open_file_name_win.h:47: OPENFILENAME* Get();
On 2014/07/31 16:27:31, sky wrote:
> nit: inline this and name it name it to match field name.

Renamed to GetOPENFILENAME(). Which seems a little unorthodox.

But GetOpenFileName would be not only confusing but probably fail to compile
because (1) that is the windows method name the structure is intended to be used
with and (2) the token is actually a macro that resolves to the 'A' or 'W'
version depending on compiler settings.

If you have a better name suggestion I can correct it in the follow-up CL.

[erikwright (departed), 2014-08-01 15:20:26]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-08-01 15:22:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/426673003/110001

[erikwright (departed), 2014-08-01 18:05:24]

Committed patchset #7 manually as r287048 (presubmit successful).