Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
449033002:
Removing unwanted canPaste() api calls and unwanted layout attributes (Closed)
|
Created:
6 years, 4 months ago by kingshuk.j Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, AKVT, Cyan, AviD, raghu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSimplify PastePopupMenu delegate interface and layout attributes
Remove redundant calls to canPaste() in PastePopupMenu. Inhibit calls to show
PastePopupMenu if clipboard is empty. Removed unwanted layout attributes to
fix the greying of the PastePopupMenu when diplayed beside the insertion
handler.
BUG=400705
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288973
Patch Set 1 #Patch Set 2 : Removed a calculation with final value #
Total comments: 3
Patch Set 3 : Merged changes from patchset for BUG=400705 #
Total comments: 2
Patch Set 4 : Replaced arrays with their singular form #
Total comments: 2
Patch Set 5 : Removed Unnecessary call to updateContent() #Patch Set 6 : Rebased patch. #Patch Set 7 : Fixed build error #Patch Set 8 : Fixed build error #
Total comments: 1
Messages
Total messages: 37 (0 generated)
@jdduke PTAL
This looks reasonable, but I have one question. https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: android.R.attr.textEditNoPasteWindowLayout, It looks like the |NoPaste| attributes go unused. cjhopman@: Do you recall what their purpose is? Do they correspond to greyed out paste buttons?
On 2014/08/07 19:25:56, jdduke wrote: > This looks reasonable, but I have one question. > > https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java > (right): > > https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: > android.R.attr.textEditNoPasteWindowLayout, > It looks like the |NoPaste| attributes go unused. cjhopman@: Do you recall what > their purpose is? Do they correspond to greyed out paste buttons? I don't know what they are.
https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: android.R.attr.textEditNoPasteWindowLayout, On 2014/08/07 19:25:56, jdduke wrote: > It looks like the |NoPaste| attributes go unused. cjhopman@: Do you recall what > their purpose is? Do they correspond to greyed out paste buttons? OK well let's get rid of these unused attributes and adjust the indexing accordingly.
https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: android.R.attr.textEditNoPasteWindowLayout, On 2014/08/07 19:37:07, jdduke wrote: > On 2014/08/07 19:25:56, jdduke wrote: > > It looks like the |NoPaste| attributes go unused. cjhopman@: Do you recall > what > > their purpose is? Do they correspond to greyed out paste buttons? > > OK well let's get rid of these unused attributes and adjust the indexing > accordingly. I have raised a bug for this greying of PastePopup due to change in the layout attributes.I have raised a bug for the same in https://codereview.chromium.org/441973002/ Shall I combine the above mentioned patch with this one? Please suggest.
On 2014/08/08 02:55:11, kingshuk.j wrote: > I have raised a bug for this greying of PastePopup due to change in the layout > attributes.I have raised a bug for the same in > https://codereview.chromium.org/441973002/ > Shall I combine the above mentioned patch with this one? Please suggest. What is the purpose of textEditSidePasteWindowLayout then if it's redundant? Do you know what the difference is?
On 2014/08/08 15:01:54, jdduke wrote: > On 2014/08/08 02:55:11, kingshuk.j wrote: > > I have raised a bug for this greying of PastePopup due to change in the layout > > attributes.I have raised a bug for the same in > > https://codereview.chromium.org/441973002/ > > Shall I combine the above mentioned patch with this one? Please suggest. > > What is the purpose of textEditSidePasteWindowLayout then if it's redundant? Do > you know what the difference is? My understanding is that, android has given different layout styles for different scenarios, and the difference between the layouts in our case can be seen as a greyish Popup.But to the end user this may seem like an improper behavior.So, i have kept only one layout style "textEditPasteWindowLayout".Changes can be found at https://codereview.chromium.org/441973002/#ps20001
On 2014/08/08 17:54:07, kingshuk.j wrote: > My understanding is that, android has given different layout styles for > different > scenarios, and the difference between the layouts in our case can be seen as a > greyish > Popup.But to the end user this may seem like an improper behavior.So, i have > kept only one layout style "textEditPasteWindowLayout".Changes can be found at > https://codereview.chromium.org/441973002/#ps20001 Please merge the changes (into this patchset), there's no advantage in having multiple patches for this update.
On 2014/08/08 18:04:12, jdduke wrote: > On 2014/08/08 17:54:07, kingshuk.j wrote: > > My understanding is that, android has given different layout styles for > > different > > scenarios, and the difference between the layouts in our case can be seen as a > > greyish > > Popup.But to the end user this may seem like an improper behavior.So, i have > > kept only one layout style "textEditPasteWindowLayout".Changes can be found at > > https://codereview.chromium.org/441973002/#ps20001 > > Please merge the changes (into this patchset), there's no advantage in having > multiple patches for this update. Merged the changes for fixing the greying of Pastepopup Menu. jdduke@ : PTAL
https://codereview.chromium.org/449033002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: mPasteViews = new View[POPUP_LAYOUT_ATTRS.length]; These should no longer be arrays, and can be made singular (mPasteView, mPasteViewLayout).
jdduke@: PTAL https://codereview.chromium.org/449033002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: mPasteViews = new View[POPUP_LAYOUT_ATTRS.length]; On 2014/08/10 19:05:05, jdduke wrote: > These should no longer be arrays, and can be made singular (mPasteView, > mPasteViewLayout). Done.
The CQ bit was checked by jdduke@chromium.org
The CQ bit was unchecked by jdduke@chromium.org
https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:132: updateContent(); I don't think we need this call any longer, right?
https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:132: updateContent(); On 2014/08/11 15:20:05, jdduke wrote: > I don't think we need this call any longer, right? Yes, I am removing this in my next patch.
On 2014/08/11 15:34:31, kingshuk.j wrote: > https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java > (right): > > https://codereview.chromium.org/449033002/diff/60001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:132: > updateContent(); > On 2014/08/11 15:20:05, jdduke wrote: > > I don't think we need this call any longer, right? > > Yes, I am removing this in my next patch. jdduke@: PTAL ,thanks.
lgtm
The CQ bit was checked by kingshuk.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kingshuk.j@samsung.com/449033002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40343) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/45585) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40349) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kingshuk.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kingshuk.j@samsung.com/449033002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was checked by kingshuk.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kingshuk.j@samsung.com/449033002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was checked by kingshuk.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kingshuk.j@samsung.com/449033002/140001
Message was sent while issue was closed.
Change committed as 288973
Message was sent while issue was closed.
https://codereview.chromium.org/449033002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/449033002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: mPasteView = null; For future reference, Java objects default to null when declared, so this is unnecessary.
Message was sent while issue was closed.
On 2014/08/12 15:26:11, jdduke wrote: > https://codereview.chromium.org/449033002/diff/140001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java > (right): > > https://codereview.chromium.org/449033002/diff/140001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:61: > mPasteView = null; > For future reference, Java objects default to null when declared, so this is > unnecessary. Thanks for the information.I will follow this in future submissions. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
