On Tue, 27 Jan 2009 21:02:29 +0100, Steve Block <[email protected]> wrote:
> Note that I've updated the patch to refer to tip-of-tree. > > ======================================================================== > http://mondrian.corp.google.com/file/9879962///depot/googleclient/gears/opensource/gears/ui/common/html_dialog.css?a=1 > File > //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.css > (snapshot 1) > ------------------------------------ > Line 112: flow and the dialog displays correctly) > Should this comment really be identical to that for Android?! > ------------------------------------ Yes, infact the comment is accurate for Opera Mobile as well but I can modify it if needed. > Line 143: ~) > The ID buttons-wince only appears in blocks guarded by IEMOBILE, so this > switch > is redundant. (I agree that buttons-wince is a slightly confusing name). > ======================================================================== You can ignore this change in that case. > http://mondrian.corp.google.com/file/9879962///depot/googleclient/gears/opensource/gears/ui/common/html_dialog.js?a=1 > File > //depot/googleclient/gears/opensource/gears/ui/common/html_dialog.js > (snapshot 1) > ------------------------------------ > Line 282: } > We've recently made changes so that this test is moved to the call site > for PIE. > I think we should do the same for Opera. Agreed. > ======================================================================== > http://mondrian.corp.google.com/file/9879962///depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4?a=1 > File > //depot/googleclient/gears/opensource/gears/ui/common/permissions_dialog.html_m4 > > (snapshot 1) > ------------------------------------ > Line 37: ~~) > Does adding an empty title on other platforms cause problems? Why is it > required > for Opera? This is because of the behaviour on Opera browser when it comes to the <title> tag. If the tag is completely absent then document.title has no effect. So in order to have a page title (in this case a dialog title) we need this tag (even though it might be empty). > ------------------------------------ > Line 261: m4_ifelse(PRODUCT_BROWSER,~IE~,m4_dnl > This isn't needed now that we switch on IEMOBILE. Agreed. > ------------------------------------ > Line 316: setButtonLabel("string-never-allow", "never-allow-button"); > There is no element with ID never-allow-button. This change can be ignored, this button existed at one point but we use the Never allow link now. > ------------------------------------ > Line 434: m4_ifelse(PRODUCT_OS,~wince~,m4_dnl > We only use m4 switches where we have to - eg in CSS. In JavaScript, you > can use > browser.opera and browser.wince - see base.js. Agreed. > ======================================================================== > http://mondrian.corp.google.com/file/9879962///depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4?a=1 > File > //depot/googleclient/gears/opensource/gears/ui/common/settings_dialog.html_m4 > > (snapshot 1) > ------------------------------------ > Line 305: if (!browser.android && !browser.opera) { > Why do you test browser.android inside a WinCE only block? In any case, > you can > use JavaScript for this test. I honestly dont know where the android check came in from. It was already there and I added Opera to the check. Possibly from a very old version of the dialog? > ======================================================================== > http://mondrian.corp.google.com/file/9879962///depot/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4?a=1 > File > //depot/googleclient/gears/opensource/gears/ui/common/shortcuts_dialog.html_m4 > > (snapshot 1) > ------------------------------------ > Line 257: ~) > Again, you can do this test using JavaScript only. Agreed. > ------------------------------------ > Line 303: ~) > Same again. > ======================================================================== > Agreed. > Regarding making modifications to the CL before submission, I'm happy > to make them on your behalf to save additional patching and > round-trips, but we need to make sure that you then pick up the > changes from tip-of-tree. Let me know how you'd like to proceed and > send me your feedback on the other comments I made. Finally, all > emails should CC [email protected]. Yes it would be good to be in sync with the tip of the tree and we'd like that to happen as soon as possible. Please make the changes necessary on my behalf and I will pick up the changes from the tip of tree for the HTML dialogs. Deepak.
