Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: desktop-options.js

Issue 29674584: Issue 5549 - Implement missing error handlings for custom filter lists (Closed)
Left Patch Set: enabled testing via ?filterError=true Created Jan. 25, 2018, 12:56 p.m.
Right Patch Set: changed custom-filters-edit-area to custom-filters-control as suggested Created Feb. 5, 2018, 9:05 a.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « desktop-options.html ('k') | locale/en_US/desktop-options.json » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-present eyeo GmbH 3 * Copyright (C) 2006-present eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
(...skipping 633 matching lines...) Expand 10 before | Expand all | Expand 10 after
644 text: findParentData(element, "access", false) 644 text: findParentData(element, "access", false)
645 }); 645 });
646 break; 646 break;
647 case "remove-subscription": 647 case "remove-subscription":
648 browser.runtime.sendMessage({ 648 browser.runtime.sendMessage({
649 type: "subscriptions.remove", 649 type: "subscriptions.remove",
650 url: findParentData(element, "access", false) 650 url: findParentData(element, "access", false)
651 }); 651 });
652 break; 652 break;
653 case "save-custom-filters": 653 case "save-custom-filters":
654 const filters = E("custom-filters-raw"); 654 const filters = E("custom-filters-raw").value;
saroyanm 2018/01/29 11:44:48 Detail/Suggestion: We don't use the element anywhe
a.giammarchi 2018/01/29 13:09:02 I'll do that, but this is nitpicking ;-)
655 const filtersValue = filters.value;
656 sendMessageHandleErrors({ 655 sendMessageHandleErrors({
657 type: "filters.importRaw", 656 type: "filters.importRaw",
658 text: filtersValue, 657 text: filters,
659 removeExisting: true 658 removeExisting: true
660 }, 659 },
661 (errors) => 660 (errors) =>
662 { 661 {
663 if (errors) 662 if (errors)
664 { 663 {
665 E("custom-filters").classList.add("warning"); 664 E("custom-filters").classList.add("warning");
666 const lines = filtersValue.split(/\n+|\n\s+\n/); 665 const customFiltersError = clearAndGetCustomFiltersError();
saroyanm 2018/01/29 11:44:48 Splitting same as it's done in the filterValidatio
a.giammarchi 2018/01/29 13:09:02 I've tested this before writing the RegExp, their
saroyanm 2018/01/29 14:30:49 I don't understand what you mean by "Logic keeps g
a.giammarchi 2018/01/29 15:01:09 if you have a textarea with a b c d the filte
saroyanm 2018/01/30 12:03:44 This doesn't work on my(Manvel) test enviroment, A
667 const info = errors.map(error => lines[error.lineno - 1]); 666
668 const editFilters = E("custom-filters-edit-filters"); 667 // The current error does not contain info about the line
669 info.forEach( 668 // that generated such error.
saroyanm 2018/01/29 11:44:48 Detail: We usualy use for..of when looping through
a.giammarchi 2018/01/29 13:09:02 have we banned the forEach from the code? performa
saroyanm 2018/01/29 14:30:49 That is a news for me. AFAIK we used for...of beca
a.giammarchi 2018/01/29 15:01:09 It's actually usually slower because it involved A
670 message => 669 // Whenever the error object will pass the bad filter
671 { 670 // within its properties, this split should be removed.
672 const li = document.createElement("li"); 671 const lines = filters.split("\n");
673 editFilters.appendChild(li).textContent = message; 672 const messages = errors.map(error => lines[error.lineno - 1]);
674 } 673 for (const message of messages)
675 ); 674 {
675 const li = document.createElement("li");
676 customFiltersError.appendChild(li).textContent = message;
677 }
676 if (errors.length > 5) 678 if (errors.length > 5)
677 editFilters.classList.add("many"); 679 customFiltersError.classList.add("many");
678 } 680 }
679 else 681 else
680 { 682 {
681 setCustomFiltersView("read"); 683 setCustomFiltersView("read");
682 } 684 }
683 }); 685 });
684 break; 686 break;
685 case "show-more-filters-section": 687 case "show-more-filters-section":
686 E("more-filters").setAttribute("aria-hidden", false); 688 E("more-filters").setAttribute("aria-hidden", false);
687 break; 689 break;
(...skipping 78 matching lines...) Expand 10 before | Expand all | Expand 10 after
766 closeDialog(); 768 closeDialog();
767 } 769 }
768 else 770 else
769 { 771 {
770 form.querySelector(":invalid").focus(); 772 form.querySelector(":invalid").focus();
771 } 773 }
772 break; 774 break;
773 } 775 }
774 } 776 }
775 777
778 function clearAndGetCustomFiltersError()
779 {
780 const customFiltersError = E("custom-filters-error");
781 customFiltersError.textContent = "";
782 customFiltersError.classList.remove("many");
783 return customFiltersError;
784 }
785
776 function setCustomFiltersView(mode) 786 function setCustomFiltersView(mode)
777 { 787 {
778 let customFiltersElement = E("custom-filters-raw"); 788 let customFiltersElement = E("custom-filters-raw");
779 updateCustomFiltersUi(); 789 updateCustomFiltersUi();
780 if (mode == "read") 790 if (mode == "read")
781 { 791 {
782 E("custom-filters").classList.remove("warning"); 792 E("custom-filters").classList.remove("warning");
783 const editFilters = E("custom-filters-edit-filters"); 793 clearAndGetCustomFiltersError();
784 editFilters.textContent = "";
785 editFilters.classList.remove("many");
786 customFiltersElement.disabled = true; 794 customFiltersElement.disabled = true;
787 if (!customFiltersElement.value) 795 if (!customFiltersElement.value)
788 { 796 {
789 setCustomFiltersView("empty"); 797 setCustomFiltersView("empty");
790 return; 798 return;
791 } 799 }
792 } 800 }
793 else if (mode == "write") 801 else if (mode == "write")
794 { 802 {
795 customFiltersElement.disabled = false; 803 customFiltersElement.disabled = false;
(...skipping 188 matching lines...) Expand 10 before | Expand all | Expand 10 after
984 type: "app.get", 992 type: "app.get",
985 what: "features" 993 what: "features"
986 }, 994 },
987 (features) => 995 (features) =>
988 { 996 {
989 hidePref("show_devtools_panel", !features.devToolsPanel); 997 hidePref("show_devtools_panel", !features.devToolsPanel);
990 }); 998 });
991 999
992 getDocLink("filterdoc", (link) => 1000 getDocLink("filterdoc", (link) =>
993 { 1001 {
994 E("link-filters").setAttribute("href", link); 1002 E("link-filters-1").setAttribute("href", link);
995 E("link-filters-on-edit-error").setAttribute("href", link); 1003 E("link-filters-2").setAttribute("href", link);
996 }); 1004 });
997 1005
998 getDocLink("subscriptions", (link) => 1006 getDocLink("subscriptions", (link) =>
999 { 1007 {
1000 E("filter-lists-learn-more").setAttribute("href", link); 1008 E("filter-lists-learn-more").setAttribute("href", link);
1001 }); 1009 });
1002 1010
1003 E("custom-filters-raw").setAttribute("placeholder", 1011 E("custom-filters-raw").setAttribute("placeholder",
1004 getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"])); 1012 getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"]));
1005 1013
(...skipping 174 matching lines...) Expand 10 before | Expand all | Expand 10 after
1180 special: true 1188 special: true
1181 }, 1189 },
1182 (subscriptions) => 1190 (subscriptions) =>
1183 { 1191 {
1184 // Load filters 1192 // Load filters
1185 for (let subscription of subscriptions) 1193 for (let subscription of subscriptions)
1186 { 1194 {
1187 browser.runtime.sendMessage({ 1195 browser.runtime.sendMessage({
1188 type: "filters.get", 1196 type: "filters.get",
1189 subscriptionUrl: subscription.url 1197 subscriptionUrl: subscription.url
1190 }, 1198 }, loadCustomFilters);
1191 (filters) =>
1192 {
1193 loadCustomFilters(filters);
1194 });
1195 } 1199 }
1196 }); 1200 });
1197 loadRecommendations(); 1201 loadRecommendations();
1198 browser.runtime.sendMessage({ 1202 browser.runtime.sendMessage({
1199 type: "prefs.get", 1203 type: "prefs.get",
1200 key: "subscriptions_exceptionsurl" 1204 key: "subscriptions_exceptionsurl"
1201 }, 1205 },
1202 (url) => 1206 (url) =>
1203 { 1207 {
1204 acceptableAdsUrl = url; 1208 acceptableAdsUrl = url;
(...skipping 293 matching lines...) Expand 10 before | Expand all | Expand 10 after
1498 }); 1502 });
1499 browser.runtime.sendMessage({ 1503 browser.runtime.sendMessage({
1500 type: "subscriptions.listen", 1504 type: "subscriptions.listen",
1501 filter: ["added", "disabled", "homepage", "lastDownload", "removed", 1505 filter: ["added", "disabled", "homepage", "lastDownload", "removed",
1502 "title", "downloadStatus", "downloading"] 1506 "title", "downloadStatus", "downloading"]
1503 }); 1507 });
1504 1508
1505 window.addEventListener("DOMContentLoaded", onDOMLoaded, false); 1509 window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
1506 window.addEventListener("hashchange", onHashChange, false); 1510 window.addEventListener("hashchange", onHashChange, false);
1507 } 1511 }
LEFTRIGHT

Powered by Google App Engine
This is Rietveld