Bug 121212 - WinLauncher needs back and forward buttons
Summary: WinLauncher needs back and forward buttons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-11 22:27 PDT by Alex Christensen
Modified: 2013-09-17 18:09 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.54 KB, patch)
2013-09-11 22:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (7.07 KB, patch)
2013-09-17 15:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (6.98 KB, patch)
2013-09-17 16:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2013-09-17 16:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2013-09-11 22:27:04 PDT
The History menu in WinLauncher is not intuitive.  Every browser and mini browser has back and forward buttons.
Comment 1 Alex Christensen 2013-09-11 22:32:00 PDT
Created attachment 211399 [details]
Patch
Comment 2 Alex Christensen 2013-09-11 22:33:06 PDT
This doesn't meet WebKit style, but it matches the style of the surrounding code.
Comment 3 WebKit Commit Bot 2013-09-11 22:34:41 PDT
Attachment 211399 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.rc']" exit_code: 1
Tools/WinLauncher/WinLauncher.cpp:368:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Tools/WinLauncher/WinLauncher.cpp:375:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Tools/WinLauncher/WinLauncher.cpp:808:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Tools/WinLauncher/WinLauncher.cpp:811:  Missing space after ,  [whitespace/comma] [3]
Tools/WinLauncher/WinLauncher.cpp:820:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Tools/WinLauncher/WinLauncher.cpp:823:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brent Fulgham 2013-09-11 22:39:30 PDT
Comment on attachment 211399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211399&action=review

Thanks for doing this. It will be ice to have these in the UI. r- so you don't delete hooks to some things I'm trying to get landed. :)

> Tools/WinLauncher/WinLauncher.cpp:90
> +LRESULT CALLBACK    MyReloadButtonProc(HWND, UINT, WPARAM, LPARAM);

Can we please call these something else? The 'My...' Naming makes it seem like a programming class project! :)

> Tools/WinLauncher/WinLauncher.cpp:379
> +                    hInst, 0);

Please format these normally with 4-space indents. One or two lines are better than these large blocks.

> Tools/WinLauncher/WinLauncher.cpp:394
> +    DefButtonProc = reinterpret_cast<WNDPROC>(GetWindowLongPtr(hBackButtonWnd, GWLP_WNDPROC));

Honestly, I'm surprised that the GetWindowLongPtr isn't defined as a 32/64 type based on compiler settings. Can you double check that this is necessary? I sort of think it could just be the ...Ptr version.

> Tools/WinLauncher/WinLauncher.cpp:-685
> -

Please don't delete this. It's WIP.

> Tools/WinLauncher/WinLauncher.cpp:-730
> -            break;

Leave in please.

> Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.rc:-52
> -    END

Leave in please.
Comment 5 Alex Christensen 2013-09-12 10:02:24 PDT
I'll finish this next week.  I've got to finish lots of homework so I can go to Tahoe this weekend :)

> > Tools/WinLauncher/WinLauncher.cpp:90
> > +LRESULT CALLBACK    MyReloadButtonProc(HWND, UINT, WPARAM, LPARAM);
> 
> Can we please call these something else? The 'My...' Naming makes it seem like a programming class project! :)
I'll rename MyEditProc, too.

> > Tools/WinLauncher/WinLauncher.cpp:394
> > +    DefButtonProc = reinterpret_cast<WNDPROC>(GetWindowLongPtr(hBackButtonWnd, GWLP_WNDPROC));
> 
> Honestly, I'm surprised that the GetWindowLongPtr isn't defined as a 32/64 type based on compiler settings. Can you double check that this is necessary? I sort of think it could just be the ...Ptr version.
If I remember correctly, there was some issue with the WinCE not recognizing the ...Ptr functions.  I think WinCE is only 32-bit and only has the 32-bit functions.  Patrick?
Comment 6 Alex Christensen 2013-09-17 15:34:05 PDT
Created attachment 211950 [details]
Patch
Comment 7 Alex Christensen 2013-09-17 16:34:23 PDT
Created attachment 211952 [details]
Patch
Comment 8 Alex Christensen 2013-09-17 16:40:09 PDT
Created attachment 211953 [details]
Patch
Comment 9 Alex Christensen 2013-09-17 16:41:54 PDT
Sorry about all the patches.  I removed duplicate calls to CallWindowProc before default and removed unnecessary casting (LRESULT to LRESULT and WNDPROC to WNDPROC)
Comment 10 Brent Fulgham 2013-09-17 17:46:41 PDT
Comment on attachment 211953 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2013-09-17 18:09:04 PDT
Comment on attachment 211953 [details]
Patch

Clearing flags on attachment: 211953

Committed r156018: <http://trac.webkit.org/changeset/156018>
Comment 12 WebKit Commit Bot 2013-09-17 18:09:07 PDT
All reviewed patches have been landed.  Closing bug.